Add a connection pool delegate #1515
Merged
+804
−36
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: It can be useful to observe what the underlying connection pool is doing over time. This could, for example, be instrumenting it to understand how many streams are active at a given time or to understand when a connection is in the processing of being brought up. Modifications: Add new API: - `GRPCConnectionPoolDelegate` which users can implement and configure on the `GRPCChannelPool` to receive notifications about the connection pool state. - `GRPCConnectionID`, an opaque ID to distinguish between different connections in the pool. Modify the connection pool: - Per-EventLoop pools now hold on to quiescing connections and track additional state, including whether a connection is quiescing and how many streams are currently open on a connection. By contrast the pool manager (which manages a number of these per-EventLoop pools) tracks reserved streams (which are not necessarily open). The delegate tracks _opened_ streams rather than _reserved_ streams (as reserved streams are not allocated to a connection but to an any connection running on an appropriate event-loop). - Wire through the new delegate and call out to it in appropriate places. - Add a stream opened function to the internal H2 delegate - Expose various pieces of state from the connection manager Result: Users can instrument the underlying connection pool.
Lukasa
reviewed
Oct 26, 2022
| // If there's a close future then the underlying channel is open. It will close eventually when | ||
| // open streams are closed, so drop the H2 delegate and update the pool delegate when that | ||
| // happens. | ||
| if let closeFuture = self._connections.values[index].manager.sync.channel?.closeFuture { |
There was a problem hiding this comment.
Holy Law of Demeter batman! We absolutely need a clearer spelling for this idea.
There was a problem hiding this comment.
Actually this whole block sniffs of abstraction violation: we repeatedly reach through several layers of objects to get to the thing we want. This suggests that the intermediate objects lack sufficient API surface to do what we want.
There was a problem hiding this comment.
Agreed. It's certainly more fiddly than I'd like and as it turns out also a little over protective: we will be notified about quiescing only if there are open streams (if there aren't then the connection will just be idled). So the else branch here is redundant. I think API for registering an on-close callback on the current connection would probably be sufficient here.
glbrntt
force-pushed
the
gb-connection-pool-delegate
branch
from
fa158e5 to
8a04180
Compare
Lukasa
reviewed
Oct 31, 2022
| /// | ||
| /// If there is a connection, the callback will be invoked with `true` when the connection is | ||
| /// closed. Otherwise the callback is invoked with `false`. | ||
| internal func onCurrentConnectionClose(_ onClose: @escaping (Bool) -> Void) { |
There was a problem hiding this comment.
This is a somewhat tricky API to use. Is there a reason it doesn't return whether the callback got registered?
There was a problem hiding this comment.
We need to hop onto the right EL first which makes this slightly awkward. A few options:
- let the caller provide a promise / return a future
- expose this via the
syncview and return whether it was registered (could be in addition to above) - use a lock in favour of the EL for synchronising here
- pass whether the callback was registered into the callback (current api)
I don't think any of these stand out as significantly better choices for an internal API. WDYT?
There was a problem hiding this comment.
Yeah, that's fair. I dunno man, it's all kinda gross. This works I suppose.
Lukasa
approved these changes
Oct 31, 2022
WendellXY
pushed a commit
to sundayfun/grpc-swift
that referenced
this pull request
Aug 24, 2023
Motivation: It can be useful to observe what the underlying connection pool is doing over time. This could, for example, be instrumenting it to understand how many streams are active at a given time or to understand when a connection is in the processing of being brought up. Modifications: Add new API: - `GRPCConnectionPoolDelegate` which users can implement and configure on the `GRPCChannelPool` to receive notifications about the connection pool state. - `GRPCConnectionID`, an opaque ID to distinguish between different connections in the pool. Modify the connection pool: - Per-EventLoop pools now hold on to quiescing connections and track additional state, including whether a connection is quiescing and how many streams are currently open on a connection. By contrast the pool manager (which manages a number of these per-EventLoop pools) tracks reserved streams (which are not necessarily open). The delegate tracks _opened_ streams rather than _reserved_ streams (as reserved streams are not allocated to a connection but to an any connection running on an appropriate event-loop). - Wire through the new delegate and call out to it in appropriate places. - Add a stream opened function to the internal H2 delegate - Expose various pieces of state from the connection manager Result: Users can instrument the underlying connection pool.
pinlin168
pushed a commit
to sundayfun/grpc-swift
that referenced
this pull request
Aug 24, 2023
Motivation: It can be useful to observe what the underlying connection pool is doing over time. This could, for example, be instrumenting it to understand how many streams are active at a given time or to understand when a connection is in the processing of being brought up. Modifications: Add new API: - `GRPCConnectionPoolDelegate` which users can implement and configure on the `GRPCChannelPool` to receive notifications about the connection pool state. - `GRPCConnectionID`, an opaque ID to distinguish between different connections in the pool. Modify the connection pool: - Per-EventLoop pools now hold on to quiescing connections and track additional state, including whether a connection is quiescing and how many streams are currently open on a connection. By contrast the pool manager (which manages a number of these per-EventLoop pools) tracks reserved streams (which are not necessarily open). The delegate tracks _opened_ streams rather than _reserved_ streams (as reserved streams are not allocated to a connection but to an any connection running on an appropriate event-loop). - Wire through the new delegate and call out to it in appropriate places. - Add a stream opened function to the internal H2 delegate - Expose various pieces of state from the connection manager Result: Users can instrument the underlying connection pool.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
It can be useful to observe what the underlying connection pool is doing over time. This could, for example, be instrumenting it to understand how many streams are active at a given time or to understand when a connection is in the processing of being brought up.
Modifications:
Add new API:
GRPCConnectionPoolDelegatewhich users can implement and configure on theGRPCChannelPoolto receive notifications about the connection pool state.GRPCConnectionID, an opaque ID to distinguish between different connections in the pool.Modify the connection pool:
Result: