-
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
Encapsulate ChangeStreamOperation #1029
Conversation
JAVA-4795
|
||
public <TResult> AsyncReadOperation<AsyncBatchCursor<TResult>> changeStream(final FullDocument fullDocument, | ||
final FullDocumentBeforeChange fullDocumentBeforeChange, final List<? extends Bson> pipeline, | ||
final Decoder<TResult> decoder, final ChangeStreamLevel changeStreamLevel, final Integer batchSize, final Collation collation, |
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.
It's odd that we have to pass in the decoder rather than just a result class, but it turns out there's slight difference in implementation between sync and async that requires this, and I didn't think this is the time to resolve the difference.
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.
I see that we also pass a decoder to SyncOperations.changeStream
for the same reason: ChangeStreamOperation
needs it. And then I see ChangeStreamOperation.getDecoder
being used in ChangeStreamBatchCursor
, which tells me that the decoder is used by both sync and async code. I don't think I understand what you are pointing to.
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.
Let me know if you want me to dig into this and explain it. I no longer remember.
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.
I don't know whether that was important or not. But given that the approach was to ignore that anyway, there is probably no reason to try and dig that out again.
|
||
public <TResult> AsyncReadOperation<AsyncBatchCursor<TResult>> changeStream(final FullDocument fullDocument, | ||
final FullDocumentBeforeChange fullDocumentBeforeChange, final List<? extends Bson> pipeline, | ||
final Decoder<TResult> decoder, final ChangeStreamLevel changeStreamLevel, final Integer batchSize, final Collation collation, |
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.
I see that we also pass a decoder to SyncOperations.changeStream
for the same reason: ChangeStreamOperation
needs it. And then I see ChangeStreamOperation.getDecoder
being used in ChangeStreamBatchCursor
, which tells me that the decoder is used by both sync and async code. I don't think I understand what you are pointing to.
Approved, but we need green tests. |
Tests are green |
JAVA-4795