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

DOCSP-31907 Split Event #457

Merged

Conversation

jordan-smith721
Copy link
Contributor

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-31907
Staging - https://docs-mongodbcom-staging.corp.mongodb.com/java/docsworker-xlarge/DOCSP-31907-split-event/fundamentals/crud/read-operations/change-streams/#split-large-change-stream-events

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?

Copy link
Contributor

@ccho-mongodb ccho-mongodb left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Just a few suggestions. Approving, but can take another look post-changes if you want.

Additional suggestion: I think this section might belong as a subheading in the "Apply Aggregation Operators to your Change Stream." What do you think?

Split Large Change Stream Events
--------------------------------

Starting in MongoDB 7.0, you can use a ``$changeStreamSplitLargeEvent``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
Use the specific article "the" since only one of them exists.

Suggested change
Starting in MongoDB 7.0, you can use a ``$changeStreamSplitLargeEvent``
Starting in MongoDB 7.0, you can use the ``$changeStreamSplitLargeEvent``


Starting in MongoDB 7.0, you can use a ``$changeStreamSplitLargeEvent``
aggregation stage to split events that exceed 16 MB into smaller fragments.
``$changeStreamSplitLargeEvent`` returns the fragments sequentially using the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions:

  • I think that the operator does not return anything, but the stage and pipeline do. Just for accuracy, I would specify what returns the fragments.
  • The style guide recommends "by using" instead of "using".
  • I think "using the change stream cursor" could be better in a separate phrase or sentence.

E.g.
"The $changeStreamSplitLargeEvent stage returns the fragments sequentially. The fragments can be accessed by using a change stream cursor."

- The index of the fragment, starting at 1

* - ``of``
- The total number of fragments in the event
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

Partially because Vale picked this up, but since "in the event" could be interpreted as an idiom, I think it could be good to reword it, and provide additional detail. E.g.

"The total number of fragments that compose the split change event

- Description

* - ``fragment``
- The index of the fragment, starting at 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
I think it could be good to indicate that "1" is a value by either using quotes or monospace.

* - ``of``
- The total number of fragments in the event

Enable the ``$changeStreamSplitLargeEvent`` aggregation on your change stream as
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think "enable the ... aggregation" might not be an accurate term to describe setting up the change stream. I think the "modify change stream output" or "control change stream output" could work better as described in the Server manual docs

E.g.
The following example modifies your change stream to split large events:

Comment on lines 327 to 328
You can access the ``SplitEvent`` by using your change stream cursor with the
``getSplitEvent()`` as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
I think sentences are easier to understand when limiting them to one association. In this sentence there are two -- "by using" and "with" -- which can make the associations ambiguous.

Suggested change
You can access the ``SplitEvent`` by using your change stream cursor with the
``getSplitEvent()`` as follows:
You can call the getSplitEvent() method on your change stream cursor to access the SplitEvent as shown in the following example:

Comment on lines 334 to 335
// Application code that creates change stream events
// ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think these lines aren't relevant to the example and should be omitted. The other lines demonstrate the objective, and it's not required for the reader to use the same app that watches for change events to also generate them.


SplitEvent event = cursor.tryNext().getSplitEvent();

For more information about accessing data from a cursor, see :ref:`java-fundamentals-cursor`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think it might be useful to either qualify this information, given that it doesn't include specific information on change stream cursors, or omit it. To qualify the information, first it would be helpful to check what similarities the two different types of cursors have and then to include that.

Hypothetically, if MongoChangeStreamCursor extended MongoCursor, I would suggest something like the following:

The MongoChangeStreamCursor extends MongoCursor and shares the same methods to access data. To learn how to access data from a MongoCursor, see ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line since it's a ChangeStreamCursor not a MongoCursor

@jordan-smith721
Copy link
Contributor Author

Additional suggestion: I think this section might belong as a subheading in the "Apply Aggregation Operators to your Change Stream." What do you think?

@ccho-mongodb I was debating on putting this under that subheading or giving it its own section. Ultimately I decided on giving it its own section because it goes a bit deeper than just describing the aggregation operator (describing a SplitEvent object and accessing the event in the cursor). I also feel like the "Apply Aggregation Operators to your Change Stream" section is more about how to apply the stage, rather than what stages are available. Happy to discuss more if needed!

@jordan-smith721 jordan-smith721 merged commit 1769d63 into mongodb:master Sep 28, 2023
2 checks passed
@jordan-smith721 jordan-smith721 requested review from a team and katcharov and removed request for a team September 28, 2023 19:34
--------------------------------

Starting in MongoDB 7.0, you can use the ``$changeStreamSplitLargeEvent``
aggregation stage to split events that exceed 16 MB into smaller fragments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth repeating the "should only use when strictly necessary" warning from the server docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in a separate PR: #460

norareidy added a commit that referenced this pull request Oct 4, 2023
* DOCSP-32942: Deprecated Methods

* review feedback

* DOCSP-33241: fix extended JSON example id field (#447)

* DOCSP-33241: fix extended JSON example id field

* DOCSP-31694: SOCKS5 proxy support (#439)

* DOCSP-31694: SOCKS5 proxy support

* DOCSP-33300: Fix aggregation operator links to the manual (#448)

* DOCSP-32300 Adds compatibility info to landing page (#449)

* DOCSP-31907 Split Event (#457)

* tech review feedback

* format

* CC feedback

* CC feedback pt 2

* DOCSP-31907 - Add paragraph from server docs (#460)

* DOCSP-32942: Deprecated Methods

* review feedback

* tech review feedback

* format

* CC feedback

* CC feedback pt 2

* build
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.

3 participants