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 MongoDB 4.0 startAtOperationTime option #306

Closed

Conversation

rwynn
Copy link

@rwynn rwynn commented Nov 27, 2018

I wasn't sure how to write new tests for this. But this is an attempt to support the new option in MongoDB 4.0 to start the change stream from a specific Timestamp in the oplog.

@rwynn
Copy link
Author

rwynn commented Jan 9, 2019

Any guidance on getting this in / testing would be appreciated. I was able to verify that it does work on 4.X but not quite sure how to write a good test / setup the data required for it.

I did notice in testing that if you do resume from a timestamp in the oplog your cannot resume from a no-op operation op = n. Resuming from other operations create/update/delete seems to work. So, for example, you can't always just pick the 1st doc in the oplog since it might be a no-op.

@eminano
Copy link

eminano commented Jan 16, 2019

Hi @rwynn ,

Sorry for the delay, it's been a busy couple of months. Thanks a lot for taking the time to implement this, it's really appreciated!

I've only had a quick look, but would it not be possible to test this by watching a collection where you insert documents at different timestamps, and using somewhere in between as your startAtOperationTime (or something to that effect), and then checking the results include the expected documents only?

I've also noticed that your code doesn't cover for the oplog restriction from the documentation when setting the startAtOperationTime value, which might be worth adding:
If the specified starting point is in the past, it must be in the time range of the oplog

Thanks again,
Esther

@rwynn
Copy link
Author

rwynn commented Jan 16, 2019

Hi @eminano, I will try to put together some tests when I get a chance. As far as the restriction is concerned, I am not sure the driver should concern itself in that detail. For example, the golang driver from MongoDB does not attempt to verify this restriction.

https://github.com/mongodb/mongo-go-driver/blob/v0.2.0/mongo/change_stream.go#L110

The use case for this that I see if that a client stores a timestamp somewhere and would like to resume change events later from that timestamp. If that timestamp passed later is invalid, the server will return an error. So I don't think the driver needs to explicitly check.

@rwynn
Copy link
Author

rwynn commented Jan 16, 2019

@eminano Actually it seems the driver should do even less validation than I am doing in this PR. The documentation states that it is invalid to pass both resumeAfter and startAtOperationTime because they are mutually exclusive. However it also states that the driver should not intervene in this case and should let the server return the error.

https://github.com/mongodb/specifications/blob/master/source/change-streams/change-streams.rst#startatoperationtime

So my PR should not be an if/elseif but rather 2 separate if checks.

@eminano
Copy link

eminano commented Jan 16, 2019

@rwynn You're right, happy to keep it simple then. Will be waiting for the tests to do a full review of the code, but it's looking good so far.
Thanks again!

@rwynn
Copy link
Author

rwynn commented Jan 16, 2019

@eminano I've updated the PR to be more in line with the spec and how MongoDB's golang driver works. Thanks!

@rwynn
Copy link
Author

rwynn commented Jan 16, 2019

@eminano I've added a test now. Thanks for your help on this.

@rwynn rwynn closed this Jan 25, 2019
@eminano
Copy link

eminano commented Jan 30, 2019

Hi @rwynn, any particular reason why you closed this PR?
Cheers,
Esther

@eminano
Copy link

eminano commented Jan 30, 2019

Sorry, I just realised you opened a new one! I'll just reference it here :)
#326

Thanks,
Esther

@rwynn
Copy link
Author

rwynn commented Jan 30, 2019

@eminano thanks and sorry for the confusion. Like you said I consolidated all the changes requested around this feature into #326.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants