Skip to content

RUST-950 Enable connection to a load balancer #415

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

Merged
merged 29 commits into from
Aug 11, 2021

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Aug 3, 2021

RUST-950

This implements the connection establishment portion of load balancer support, i.e.:

  • Parsing the loadBalanced URI/TXT option
  • Modifications to handshake and topology/server type tracking
  • Per-serviceId connection pool tracking

The work here happened to also encompass RUST-955.

// TODO RUST-653 Remove this when load balancer work is ready for release.
#[builder(default, setter(skip))]
#[serde(skip)]
pub(crate) allow_load_balanced: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This awkward double-flag construction allows load_balanced to be populated by the URI/TXT parsing as it will be on release, but prevents that from being used outside of tests that have access to set allow_load_balanced to be true.

@abr-egn abr-egn marked this pull request as ready for review August 3, 2021 19:39
@abr-egn abr-egn force-pushed the RUST-950/load-balancer-connection branch from 4f2644a to afd1b72 Compare August 6, 2021 18:39
src/srv.rs Outdated
@@ -189,11 +191,28 @@ impl SrvResolver {
"replicaset" => {
config.replica_set = Some(parts[1].into());
}
"loadBalanced" => {
Copy link

@magicalcookie magicalcookie Aug 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be lowercase. See Line 187.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

@abr-egn abr-egn marked this pull request as draft August 9, 2021 13:54
@abr-egn
Copy link
Contributor Author

abr-egn commented Aug 9, 2021

This has broken all tests with auth enabled; converted to draft while I track that down.

@abr-egn abr-egn marked this pull request as ready for review August 9, 2021 15:45
@abr-egn
Copy link
Contributor Author

abr-egn commented Aug 9, 2021

This has broken all tests with auth enabled; converted to draft while I track that down.

Ah, found it; I'd broken HandshakePhase::is_before_completion. Good to review.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just have a few minor suggestions / questions.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny nitpick otherwise LGTM!

@abr-egn abr-egn force-pushed the RUST-950/load-balancer-connection branch from c340c54 to 78bfb11 Compare August 11, 2021 17:33
@abr-egn abr-egn merged commit 2b7175e into mongodb:master Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants