-
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
Implement Java sync improved bulk write API and unified spec tests #1486
Conversation
…nstructor JAVA-5527
JAVA-5527
JAVA-5527
JAVA-5527
JAVA-5528
…lientWriteModel` subtypes JAVA-5527
JAVA-5528
JAVA-5528
…fic concrete types JAVA-5527
… not vice versa JAVA-5527
…ould extend each other Such extending turned out to be confusing JAVA-5527
JAVA-5528
…` public JAVA-5527
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.
Looks really good.
Just had a couple of suggestions. Less worried about the TODO's but I do recommend summarizing the slack discussion in the comment.
final Runnable ifCommandIsRetryable) { | ||
// BULK-TODO This implementation must limit the number of `models` it includes in a batch if needed. | ||
// Each batch re-selects a server and re-checks out a connection because this is simpler and it is allowed, | ||
// see https://mongodb.slack.com/archives/C035ZJL6CQN/p1722265720037099?thread_ts=1722264610.664109&cid=C035ZJL6CQN. |
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.
Nit: These archives will be lost over time, so recommend adding a tldr here 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.
Done in 14bb86e. I moved the comment, and linked the relevant DRIVERS
ticket there (the ticket will not be done, but it has the relevant discussion).
...ync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java
Show resolved
Hide resolved
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
} | ||
}, | ||
{ | ||
"name": "bulkWrite", |
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.
Shouldn't this be clientBulkWrite
? When I run locally this test fails looking for a collection named client
.
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.
Thank you, it should. It haven't noticed this because I've been only running tests against a replica set, where this test is skipped. This is how it's in the spec, so I'll file a relevant DRIVERS ticket
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.
Created https://jira.mongodb.org/browse/DRIVERS-2978 with a PR.
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.
@jyemin Could you please move https://jira.mongodb.org/browse/DRIVERS-2978 to "Ready for work"? I don't have the permission to do that.
…te operation: log messages have operationIds` JAVA-5528
JAVA-5528
…WriteModel.insertOne` JAVA-5527
…riteResult.java Co-authored-by: Viacheslav Babanin <[email protected]>
…eResult.java Co-authored-by: Viacheslav Babanin <[email protected]>
JAVA-5527
…he outer class name JAVA-5527
…oseResults` JAVA-5527
JAVA-5527 DRIVERS-2975
JAVA-5528
The first three commits are well-organized to allow reviewing them one by one.
This PR depends on #1458.
The following test runners execute the unified and prose tests added in this PR:
com.mongodb.client.unified.VersionedApiTest
com.mongodb.client.unified.UnifiedTransactionsTest
com.mongodb.client.unified.ServerSelectionLoggingTest
com.mongodb.client.unified.UnifiedRetryableWritesTest
com.mongodb.client.unified.CommandMonitoringTest
com.mongodb.client.unified.UnifiedCrudTest
com.mongodb.client.CrudProseTest
JAVA-5528
JAVA-5609