Refactor MixedBulkWriteOperation a bit and change CommandMessage.isResponseExpected such that it accounts for ordered/unordered bulk writes
#1481
Conversation
| if (sessionContext.hasSession() && !sessionContext.isImplicitSession() && !sessionContext.hasActiveTransaction() | ||
| && !writeConcern.isAcknowledged()) { | ||
| throw new MongoClientException("Unacknowledged writes are not supported when using an explicit session"); | ||
| } |
There was a problem hiding this comment.
This validation was moved to MixedBulkWriteOperation.validateAndGetEffectiveWriteConcern.
| if (!writeConcern.isServerDefault() && !sessionContext.hasActiveTransaction()) { | ||
| command.put("writeConcern", writeConcern.asDocument()); | ||
| } |
There was a problem hiding this comment.
This logic needs to be reused in the implementation of the new bulkWrite command, so I extracted it into MixedBulkWriteOperation.commandWriteConcern.
There was a problem hiding this comment.
Will it eventually move to some common "helper" class?
There was a problem hiding this comment.
I had no such plans, as I don't see any value in creating a somewhat useless class just to hold this method, such that both MixedBulkWriteOperation and ClientBulkWriteOperation could call commandWriteConcern on that class. Calling commandWriteConcern on MixedBulkWriteOperation seems to be not worse in any way, especially given that when it is statically imported, one can't even notice immediately see where the method comes from.
| private boolean shouldExpectResponse(final BulkWriteBatch batch, final WriteConcern effectiveWriteConcern) { | ||
| return effectiveWriteConcern.isAcknowledged() || (ordered && batch.hasAnotherBatch()); | ||
| } |
There was a problem hiding this comment.
shouldExpectResponse replaces the shouldAcknowledge method. The new method has simpler body and named in a way that avoids erroneous allusions to an unacknowledged (w: 0) write concern.
| @@ -253,7 +253,7 @@ private boolean requireOpMsgResponse() { | |||
| if (responseExpected) { | |||
There was a problem hiding this comment.
We can simplify this class bit further by adding the following assertion in the constructor:
assertFalse(getOpCode() == OpCode.OP_QUERY && !responseExpected);
this assertion should never fire because OP_QUERY effectively is only used for the initial hello command on a new connection, which should always be expecting a response.
Then we can remove the !useOpMsg() condition in isResponseExpected, and inline this method.
| if (!writeConcern.isServerDefault() && !sessionContext.hasActiveTransaction()) { | ||
| command.put("writeConcern", writeConcern.asDocument()); | ||
| } |
There was a problem hiding this comment.
Will it eventually move to some common "helper" class?
stIncMale
changed the title
MixedBulkWriteOperation a bit and change CommandMessage.requireOpMsgResponse such that it accounts for ordered/unordered bulk writesMixedBulkWriteOperation a bit and change CommandMessage.isResponseExpected such that it accounts for ordered/unordered bulk writes
| @@ -350,9 +351,11 @@ class MongoClientSessionSpecification extends FunctionalSpecification { | |||
|
|
|||
| void waitForInsertAcknowledgement(MongoCollection<Document> collection, ObjectId id) { | |||
| Document document = collection.find(Filters.eq(id)).first() | |||
| Timeout timeout = Timeout.expiresIn(5, TimeUnit.SECONDS, Timeout.ZeroSemantics.ZERO_DURATION_MEANS_INFINITE) | |||
There was a problem hiding this comment.
Is this change relevant to the rest of the PR?
There was a problem hiding this comment.
Sorry, I thought I posted my comments, but they were pending. Yes, here are the relevant comments:
- Refactor
MixedBulkWriteOperationa bit and changeCommandMessage.isResponseExpectedsuch that it accounts for ordered/unordered bulk writes #1481 (comment) - Refactor
MixedBulkWriteOperationa bit and changeCommandMessage.isResponseExpectedsuch that it accounts for ordered/unordered bulk writes #1481 (comment)
| void waitForInsertAcknowledgement(MongoCollection<Document> collection, ObjectId id) { | ||
| Document document = collection.find(Filters.eq(id)).first() | ||
| Timeout timeout = Timeout.expiresIn(5, TimeUnit.SECONDS, Timeout.ZeroSemantics.ZERO_DURATION_MEANS_INFINITE); | ||
| while (document == null) { | ||
| Thread.sleep(1) | ||
| document = collection.find(Filters.eq(id)).first() | ||
| timeout.onExpired {throw new RuntimeException("Timed out waiting for insert acknowledgement")} | ||
| } | ||
| } |
There was a problem hiding this comment.
When playing with the assertion added to SplittablePayload.hasAnotherSplit, I noticed that this method makes the tests hang if assertion error happens. So I introduced a timeout. When the assertion error is thrown and timeout expires, the assertion error is observable in the stack trace of the exception thrown by the test (I manually checked that).
| @@ -253,7 +253,7 @@ private boolean requireOpMsgResponse() { | |||
| if (responseExpected) { | |||
| while (document == null) { | ||
| Thread.sleep(1) | ||
| document = collection.find(Filters.eq(id)).first() | ||
| timeout.onExpired { assert !"Timed out waiting for insert acknowledgement".trim() } |
There was a problem hiding this comment.
The checker is not happy when I throw RuntimeException, so I decided to use Spock's assert. However, it does not take a message, so I used a negation of the error message to emulate the failure message. But then the checker wasn't happy because it could see that the assertion outcome is predetermined, which is why I had to use trim (toString did not fool the checker).
No description provided.