refactor(bigquery/storage/managedwriter): augment connection pool #7174
Conversation
This file contains 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
This PR adds two references to live code: * a pool reference on ManagedStream * a reference to ManagedStream on pendingWrite The ManagedStream->pool reference is to allow a writer to resolve where to find its associated connection, retries, lookups, etc. The reference on the pendingWrite is primarily in service of retries, particularly when we need to re-enqueue and thus potentially re-resolve what connection is associated with the writer. This PR also moves some of the retry processing code onto the connectionPool in service to that goal. As before, this is new code that isn't yet referenced from existing functionality. This PR also more substantially starts to carve out connection management in the pool, providing a basic connection resolver and eviction capabilities. This initial implementation is primitive, and aligns with our current behavior (single unshared connection per writer). We also add some testing of the mapping behavior to ensure we're consistently updating the map for resolution and eviction. Towards: googleapis#7103
product-auto-label
bot
added
size: m
product-auto-label
bot
added
size: l
alvarowolfx
reviewed
Dec 28, 2022
| @@ -148,6 +298,68 @@ func (co *connection) getStream(arc *storagepb.BigQueryWrite_AppendRowsClient, f | |||
| return co.arc, co.pending, co.err | |||
| } | |||
|
|
|||
| // lockingAppend handles a single append request on a connection. | |||
| func (c *connection) lockingAppend(pw *pendingWrite) error { | |||
There was a problem hiding this comment.
are you planning to remove the lockingAppend method on both connection and ManagedStream on this PR or you are going to remove from ManagedStream already ?
There was a problem hiding this comment.
When I reach the refactor point where I modify ManagedStream most of the functionality goes away, lockingAppend and other connection-oriented funcs will be removed.
|
Closing this, taking a slightly different approach now. |
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.
This PR adds two references to live code:
The ManagedStream->pool reference is to allow a writer to resolve where to find its associated connection, retries, lookups, etc.
The reference on the pendingWrite is primarily in service of retries, particularly when we need to re-enqueue and thus potentially re-resolve what connection is associated with the writer. This PR also moves some of the retry processing code onto the connectionPool in service to that goal. As before, this is new code that isn't yet referenced from existing functionality.
This PR also more substantially starts to carve out connection management in the pool, providing a basic connection resolver and eviction capabilities. This initial implementation is primitive, and aligns with our current behavior (single unshared connection per writer).
The existing lockingAppend behavior on ManagedStream is added to the connection abstraction. We do not move appendWithRetries as that should stay with the writer (we may want to retry using a different connection).
We also add some testing of the mapping behavior to ensure we're consistently updating the map for resolution and eviction.
Towards: #7103