-
Notifications
You must be signed in to change notification settings - Fork 183
RUST-2131 Fix bulk write cursor iteration on load balanced topologies #1358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -403,30 +407,6 @@ impl Client { | |||
implicit_session, | |||
}, | |||
Err(mut err) => { | |||
// If the error is a reauthentication required error, we reauthenticate and | |||
// retry the operation. | |||
if err.is_reauthentication_required() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be moved outside of this retry loop so that reauthentication can be done, if needed, when we're calling getMore directly with execute_operation_on_connection
. The main functional difference is that we're now retrying the operation on the same connection rather than going through server selection/connection checkout again. The auth spec doesn't require either of these paths, and I think it's reasonable to reuse the freshly-reauthenticated connection in this case.
@@ -484,327 +464,275 @@ impl Client { | |||
} | |||
|
|||
/// Executes an operation on a given connection, optionally using a provided session. | |||
async fn execute_operation_on_connection<T: Operation>( | |||
pub(crate) async fn execute_operation_on_connection<T: Operation>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest hiding whitespace changes in the diff to reduce noise here
@@ -837,6 +765,98 @@ impl Client { | |||
}) | |||
} | |||
|
|||
async fn reauthenticate_connection(&self, connection: &mut PooledConnection) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no functional changes in the contents of these methods, just moved them for readability
self.execute_operation_with_retry(op, session).await | ||
}) | ||
.await | ||
// If the current transaction has been committed/aborted and it is not being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having this here instead of in execute_operation_with_retry
- it makes the purpose of that method cleaner - but for my own understanding, was there a functional reason for this change beyond that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, this and the reshuffling above were to simplify execute_operation_with_retry
and execute_operation_on_connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a possible suggestion and assert removal.
RUST-2131