-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
BatchCursor refactorings #1246
BatchCursor refactorings #1246
Conversation
e4008aa
to
436d236
Compare
- Added SingleBatchCursor - QueryResult and QueryBatchCursor renaming - Renamed and moved QueryResult to CommandCursorResult - Renamed QueryBatchCursor to CommandBatchCursor - Renamed AsyncQueryBatchCursor to AsyncCommandBatchCursor - Unified Async & Sync CommandBatchCursor testing - Added a CursorResourceManager for both Async & Sync JAVA-5159
436d236
to
ca023dc
Compare
@jyemin I've ported the cursor changes to master. However, I'm struggling with the Loadbalancer tests - seems lots are failing. Any ideas on these or how I can test them locally? |
I no longer remember how to start up a load balancer locally. You might need to look at how it's done in Evergreen and emulate it. Happy to look at the failures on Evergreen if you point me to them. |
@jyemin See: https://spruce.mongodb.com/version/654a27dd0305b9e381425fdf/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC I got haproxy working but the driver throws an error saying the server does not support it. |
Did you find https://github.com/mongodb-labs/drivers-evergreen-tools/blob/master/.evergreen/orchestration/configs/sharded_clusters/basic-load-balancer.json? That will show how to configure mongos to support LB mode. |
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.
What did you end up having to do to get load balancer mode to work again?
@@ -79,7 +79,7 @@ echo $second | |||
-Dorg.mongodb.test.uri=${SINGLE_MONGOS_LB_URI} \ | |||
-Dorg.mongodb.test.multi.mongos.uri=${MULTI_MONGOS_LB_URI} \ | |||
${GRADLE_EXTRA_VARS} --stacktrace --info --continue driver-core:test \ | |||
--tests QueryBatchCursorFunctionalSpecification | |||
--tests CommandBatchCursorSpecification |
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.
CommandBatchCursorSpecification
is a unit test. Do we want CommandBatchCursorFunctionalTest
instead?
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.
Good catch - will update.
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.
Updated
driver-core/src/main/com/mongodb/internal/operation/AsyncCommandBatchCursor.java
Show resolved
Hide resolved
@@ -71,7 +71,9 @@ public void shouldCreateServerSessionOnlyAfterConnectionCheckout() throws Interr | |||
.addCommandListener(new CommandListener() { | |||
@Override | |||
public void commandStarted(final CommandStartedEvent event) { | |||
lsidSet.add(event.getCommand().getDocument("lsid")); | |||
if (event.getCommand().containsKey("lsid")) { |
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.
What problem did this address? I can't think of a command that wouldn't have an lsid on it.
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.
com.mongodb.reactivestreams.client.SessionsProseTest.shouldIgnoreImplicitSessionIfConnectionDoesNotSupportSessions
failed on 4.2, 5.0, 6.0 due to this erroring.
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.
That seems impossible, since this code change only effects the shouldCreateServerSessionOnlyAfterConnectionCheckout
test in which it is contained. I think something else must be happening that caused shouldIgnoreImplicitSessionIfConnectionDoesNotSupportSessions
test to fail.
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.
[2023/11/07 18:16:39.381] 18:13:54.022 [Test worker] WARN org.mongodb.driver.protocol.event - Exception thrown raising command started event to listener com.mongodb.client.AbstractSessionsProseTest$1@36ad5f2a
[2023/11/07 18:16:39.381] org.bson.BsonInvalidOperationException: Document does not contain key lsid
[2023/11/07 18:16:39.381] at org.bson.BsonDocument.throwIfKeyAbsent(BsonDocument.java:883) ~[bson-4.12.0-SNAPSHOT.jar:na]
[2023/11/07 18:16:39.381] at org.bson.BsonDocument.getDocument(BsonDocument.java:153) ~[bson-4.12.0-SNAPSHOT.jar:na]
[2023/11/07 18:16:39.381] at com.mongodb.client.AbstractSessionsProseTest$1.commandStarted(AbstractSessionsProseTest.java:74) ~[test/:na]
[2023/11/07 18:16:39.381] at com.mongodb.internal.connection.ProtocolHelper.sendCommandStartedEvent(ProtocolHelper.java:283) ~[mongodb-driver-core-4.12.0-SNAPSHOT.jar:na]
[2023/11/07 18:16:39.381]
However, looks like there was also an exception opening socket. Which maybe the root cause.
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.
Reverting
This reverts commit 287a25e.
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!
JAVA-5159