Skip to content
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

Merged
merged 5 commits into from Nov 9, 2023
Merged

BatchCursor refactorings #1246

merged 5 commits into from Nov 9, 2023

Conversation

rozza
Copy link
Member

@rozza rozza commented Nov 6, 2023

  • 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

@rozza rozza marked this pull request as draft November 6, 2023 15:52
@rozza rozza force-pushed the JAVA-5159-master branch 2 times, most recently from e4008aa to 436d236 Compare November 7, 2023 11:27
- 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
@rozza
Copy link
Member Author

rozza commented Nov 7, 2023

@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?

@jyemin
Copy link
Contributor

jyemin commented Nov 7, 2023

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.

@rozza
Copy link
Member Author

rozza commented Nov 7, 2023

@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.

@jyemin
Copy link
Contributor

jyemin commented Nov 7, 2023

@rozza rozza marked this pull request as ready for review November 8, 2023 12:14
@rozza
Copy link
Member Author

rozza commented Nov 8, 2023

@jyemin all tests pass!

Couple of commits not PR'd please see 314e2ae and 287a25e

Copy link
Contributor

@jyemin jyemin left a 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
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch - will update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@@ -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")) {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@jyemin jyemin Nov 8, 2023

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.

Copy link
Member Author

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.

https://parsley.mongodb.com/test/mongo_java_driver_tests_jdk8_unsecure__version~4.2_os~linux_topology~standalone_auth~noauth_ssl~nossl_jdk~jdk8_test_patch_616ab9d32470d5f9aec3abdd3c3664256c49f52f_654a7c0cc9ec44fde9670c48_23_11_07_18_03_57/0/7b211511de7ffca6cd7363742c881993?bookmarks=0,247&shareLine=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverting

@rozza rozza requested a review from jyemin November 8, 2023 15:06
Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

LGTM!

@rozza rozza merged commit 540c612 into mongodb:master Nov 9, 2023
50 of 56 checks passed
@rozza rozza deleted the JAVA-5159-master branch November 9, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants