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

Support $changeStreamSplitLargeEvent #1159

Merged
merged 6 commits into from
Aug 4, 2023
Merged

Conversation

katcharov
Copy link
Contributor

@katcharov katcharov requested review from stIncMale and vbabanin July 13, 2023 19:04
// #. Create a change stream _S_ by calling ``watch`` on _C_ with
// pipeline ``[{ "$changeStreamSplitLargeEvent": {} }]`` and ``fullDocumentBeforeChange=required``.
ChangeStreamIterable<Document> changeStream = collection
.watch(asList(Document.parse("{ $changeStreamSplitLargeEvent: {} }")))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not create a stage builder, since we do not seem to offer stages for other stages like this (for example, $currentOp).

Comment on lines +137 to +138
@Deprecated
public ChangeStreamDocument(@BsonProperty("operationType") final String operationTypeString,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unfortunate proliferation of constructors, but it seems in line with what was done previously in this class.

Copy link
Member

Choose a reason for hiding this comment

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

Should we include a "@deprecated" tag to specify the reason for deprecation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the following just now,

     * @deprecated Prefer a non-deprecated constructor

but when I read it, it seemed too obvious, like "@deprecated use something that isn't deprecated". I think one convention (apart from not including the tag) is to link to the preferred replacement, but here, I do not think we should do this since there have already been N replacements, so it seemed like a link would become outdated.

If you still think this is worth including, I don't feel too strongly, but let me know the wording.

* @since 4.7
*/
@Deprecated
public ChangeStreamDocument(@BsonProperty("operationType") final String operationTypeString,
Copy link
Member

Choose a reason for hiding this comment

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

Before the server change supported in this PR, the operationType field of a ChangeStreamDocument (it is called a "change event" in the MongoDB docs) was not optional: https://github.com/mongodb/specifications/blob/master/source/change-streams/change-streams.rst#response-format, and was set in all events (to see that one has to click and look through all events documented in https://www.mongodb.com/docs/v7.0/reference/change-events/, which is, no doubt, convenient). However, that field appear to become optional (a breaking change 🤷‍♂️) since MongoDB 7.0 based on the comments in https://jira.mongodb.org/browse/DRIVERS-2617. Given that this is not documented anywhere (neither in https://www.mongodb.com/docs/v7.0/reference/operator/aggregation/changeStreamSplitLargeEvent/ or https://www.mongodb.com/docs/v7.0/reference/operator/aggregation/changeStream/, nor in https://github.com/mongodb/specifications/blob/master/source/change-streams/change-streams.rst, nor anywhere in https://www.mongodb.com/docs/v7.0/reference/change-events/), this should be clarified. If the field indeed becomes optional, we will need to add @Nullable annotations appropriately to ChangeStreamDocument, and either change the documentation of OperationType.OTHER, or add a new OMITTED value.1


1 Bizarrely, OperationType.fromString already handles null references (they correspond to missing operationType fields), even though they could not have been missing, and if they were missing, that would have rendered ChangeStreamDocument.getOperationTypeString broken, because the method must not return null (note how ChangeStreamDocument methods that may return null are annotated with @Nullable, for example, getFullDocument).

Copy link
Member

Choose a reason for hiding this comment

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

Based on https://mongodb.slack.com/archives/C72LB5RPV/p1689609952334209, it looks like the work required to start supporting $changeStreamSplitLargeEvent in drivers was not done, even though it was supposed to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll pause on this part until the changes are completed.

Copy link
Member

Choose a reason for hiding this comment

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

Should I resolve this, or is there something you are still waiting for?

@@ -103,14 +107,52 @@ public ChangeStreamDocument(@BsonProperty("operationType") final String operatio
this.fullDocument = fullDocument;
this.clusterTime = clusterTime;
this.operationTypeString = operationTypeString;
this.operationType = OperationType.fromString(operationTypeString);
this.operationType = operationTypeString == null ? null : OperationType.fromString(operationTypeString);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fromString does not take null; note that it is used in the OperationTypeCodec

Copy link
Member

Choose a reason for hiding this comment

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

I am glad we are not passing null to OperationType.fromString, even though it (arguably, incorrectly) has the code to handle nulls.

@katcharov katcharov requested a review from stIncMale July 18, 2023 22:03
@@ -103,14 +107,52 @@ public ChangeStreamDocument(@BsonProperty("operationType") final String operatio
this.fullDocument = fullDocument;
this.clusterTime = clusterTime;
this.operationTypeString = operationTypeString;
this.operationType = OperationType.fromString(operationTypeString);
this.operationType = operationTypeString == null ? null : OperationType.fromString(operationTypeString);
Copy link
Member

Choose a reason for hiding this comment

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

I am glad we are not passing null to OperationType.fromString, even though it (arguably, incorrectly) has the code to handle nulls.

@katcharov katcharov requested a review from stIncMale July 24, 2023 17:40
Comment on lines +137 to +138
@Deprecated
public ChangeStreamDocument(@BsonProperty("operationType") final String operationTypeString,
Copy link
Member

Choose a reason for hiding this comment

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

Should we include a "@deprecated" tag to specify the reason for deprecation?

@katcharov katcharov requested a review from vbabanin July 26, 2023 15:49
*
* @return the split event
* @since 4.11
* @mongodb.server.release 7.0
Copy link
Member

@vbabanin vbabanin Jul 27, 2023

Choose a reason for hiding this comment

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

Since SplitEvent works with 6.0.9 and the documentation has been updated for it, let's change it here to 6.0.9 as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

@katcharov katcharov requested a review from vbabanin August 3, 2023 15:10
@katcharov katcharov merged commit e4f39e1 into mongodb:master Aug 4, 2023
@katcharov katcharov deleted the JAVA-4955 branch August 4, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants