search

globalsign / mgo

The MongoDB driver for Go
Other
1.97k stars 230 forks source link

support MongoDB 4.0 startAtOperationTime option #306

Closed rwynn closed 5 years ago

rwynn commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

rwynn commented 5 years ago

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

eminano commented 5 years ago

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

eminano commented 5 years ago

Sorry, I just realised you opened a new one! I'll just reference it here :) https://github.com/globalsign/mgo/pull/326

Thanks, Esther

rwynn commented 5 years ago

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