RUST-980 Run load balancer tests on evergreen, and update existing tests #477
Conversation
| @@ -313,6 +313,7 @@ impl Connection { | |||
| let (tx, rx) = mpsc::channel(1); | |||
| self.pinned_sender = Some(tx); | |||
| Ok(PinnedConnectionHandle { | |||
| id: self.id, | |||
There was a problem hiding this comment.
This isn't needed for functionality, but it's very handy for debugging.
| @@ -78,6 +78,10 @@ async fn concurrent_connections() { | |||
| let _guard = LOCK.run_exclusively().await; | |||
|
|
|||
| let mut options = CLIENT_OPTIONS.clone(); | |||
| if options.load_balanced.unwrap_or(false) { | |||
There was a problem hiding this comment.
direct_connection is explicitly incompatible with load_balanced.
| @@ -513,13 +513,11 @@ impl ConnectionPoolWorker { | |||
| handler.handle_pool_cleared_event(event); | |||
| }); | |||
|
|
|||
| if !self.generation.is_load_balanced() { | |||
There was a problem hiding this comment.
I swear that some part of the spec said that the queue shouldn't be drained in load-balancing mode, but I was apparently hallucinating that.
There was a problem hiding this comment.
Did this draining behavior cause tests to fail? Intuitively, it makes sense to not drain in LB mode, since these requests will probably go to a working service rather than the one that caused a clear.
There was a problem hiding this comment.
Yep - https://github.com/mongodb/mongo-rust-driver/blob/master/src/test/spec/retryable_reads.rs#L74 failed (the second checkout succeeds rather than fails), and the load balancer spec specifically calls out retryable reads tests.
There was a problem hiding this comment.
Ah okay, so that test is specifically verifying the pool paused behavior, which shouldn't apply to load balancers. The spec doesn't currently clarify this so I filed DRIVERS-1942 to update it. In the meantime, I think we can preserve the existing behavior of not clearing the WaitQueue on pool clear if in load balanced mode and just skip that individual test.
There was a problem hiding this comment.
Updated - needed to skip the equivalent test for retryable writes as well.
| } | ||
|
|
||
| pub fn is_sharded(&self) -> bool { | ||
| self.server_info.msg.as_deref() == Some("isdbgrid") |
There was a problem hiding this comment.
This returns true for load-balanced toplogies, so to avoid having to manually maintain a bunch of fiddly boolean exclusions I refactored these.
| @@ -513,13 +513,11 @@ impl ConnectionPoolWorker { | |||
| handler.handle_pool_cleared_event(event); | |||
| }); | |||
|
|
|||
| if !self.generation.is_load_balanced() { | |||
There was a problem hiding this comment.
Did this draining behavior cause tests to fail? Intuitively, it makes sense to not drain in LB mode, since these requests will probably go to a working service rather than the one that caused a clear.
| @@ -185,13 +190,20 @@ impl Handshaker { | |||
| if options.load_balanced { | |||
| command.body.insert("loadBalanced", true); | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| { | |||
There was a problem hiding this comment.
(just out of curiosity) why does this statement need to be in its own scope?
There was a problem hiding this comment.
Applying attributes to individual statements turns out not to be available in stable Rust yet.
abr-egn
force-pushed
the
RUST-980/lb-existing-tests
branch
from
e55cb94 to
afe3a39
Compare
RUST-980
This adds evergreen configuration to run the tests against a load-balancer, updates several existing tests to run on that topology, and fixes several bugs that those tests revealed.
A few unrelated changes got picked up in test syncs: serverless run configuration (which the runner already understood) and language changes.