RUST-663 Support $merge and $out executing on secondaries #1360
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
abr-egn
commented
Apr 17, 2025
src/client/executor.rs
Outdated
| @@ -651,78 +661,9 @@ impl Client { | |||
| let start_time = Instant::now(); | |||
| let command_result = match connection.send_message(message).await { | |||
| Ok(response) => { | |||
| async fn handle_response<T: Operation>( | |||
There was a problem hiding this comment.
I pulled this out and renamed it for readability, but the body is unchanged.
There was a problem hiding this comment.
ha, I also did this in #1358 (minus the rename)
| @@ -205,7 +205,7 @@ impl TopologyDescription { | |||
| &self, | |||
| address: &ServerAddress, | |||
| command: &mut Command, | |||
| criteria: Option<&SelectionCriteria>, | |||
| criteria: &SelectionCriteria, | |||
There was a problem hiding this comment.
Another side effect: this method no longer receives an Option, so it could be slightly simplified.
isabelatkinson
approved these changes
Apr 21, 2025
src/client/executor.rs
Outdated
| @@ -651,78 +661,9 @@ impl Client { | |||
| let start_time = Instant::now(); | |||
| let command_result = match connection.send_message(message).await { | |||
| Ok(response) => { | |||
| async fn handle_response<T: Operation>( | |||
There was a problem hiding this comment.
ha, I also did this in #1358 (minus the rename)
abr-egn
force-pushed
the
RUST-663/aggregation-read-pref
branch
from
6c0c335 to
50c75de
Compare
|
Rebase required a little bit of rewriting, so please do another look over. |
isabelatkinson
approved these changes
Apr 28, 2025
There was a problem hiding this comment.
LGTM. we might not be able to merge this til the cargo-deny error is fixed because of the rules I added for RUST-1907
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.
RUST-663
Attempt 1: add a new method on
Operationthat could update the stored selection criteria before server selection given a topology snapshot. Didn't work because server selection might need to wait for handshake to finish and if so the snapshot won't have the version information.Attempt 2: Update
Client::select_serverto take anOperationrather than a string operation name, but it's called from various contexts that don't have an operation to give which would need to synthesize one, and that seemed worse.Attempt 3 (this PR): Plumb a single callback from the
Operationintoselect_server(no-op by default) to override selection criteria in the inner loop of server selection, based on the always-most-recent view of topology.As a side effect, I factored
Commandconstruction out into its own function so that it was easier to record the final effective selection criteria rather than the staticOperation-associated criteria; I think this makes the inner portion of command execution a bit easier to read and makes it easier to keep track of what's going on between the various flavors ofexecute_whatever_with_whateverin there.