RUBY-1228 Change Streams

[estolfo]

No description provided.

[saghm]

I looked over this, and unfortunately I don't really have any useful feedback to provide. Everything seems fine to me.

[estolfo]

ok, thank you @saghm !

[ShaneHarvey]

I don't think getmores should be retryed with the same cursor. The spec says to create a new cursor on a retryable error: https://github.com/mongodb/specifications/blob/master/source/change-streams.rst#resume-process

[estolfo]

Right, the cursor created by the ChangeStream has the options { disable_retry: true} so the first block that doesn't wrap the get_more in the read_with_retry block will be executed.

[ShaneHarvey]

Thanks for the explanation.

[ShaneHarvey]

I have a couple questions. First, the spec says:

A driver SHOULD attempt to kill the cursor on the server on which the cursor is opened during the resume process, and MUST NOT attempt to kill the cursor on any other server.

Does close ensure that the kill cursor request goes to the original server?

Second, create_cursor retrys once and this method retrys as well. That means it will retry on consecutive network errors. The spec says to only retry on the first network error. Any errors that happen during the retry should be fatal. I think instead of (@cursor || create_cursor!) it should be something like (@cursor || create_cursor_no_retry!). Do you agree or am I misunderstanding the code?

[estolfo]

Does close ensure that the kill cursor request goes to the original server?

Yes. A cursor has a reference to the server (@server variable). The kill cursors method called on the cursor in line 105, sends the operation to that server.

Second, create_cursor retrys once and this method retrys as well. That means it will retry on consecutive network errors. The spec says to only retry on the first network error. Any errors that happen during the retry should be fatal. I think instead of (@cursor || create_cursor!) it should be something like (@cursor || create_cursor_no_retry!). Do you agree or am I misunderstanding the code?

Yes, you're right. I shouldn't be retrying in the create_cursor method. I altered the logic a few commits ago. I believe it was correct before so I'll change it back.

[ShaneHarvey]

I thought I remember reading that the initial aggregation command should not be retryed but I don't see it in the spec. I'll open a spec question.

[ShaneHarvey]

Waiting for an answer: https://jira.mongodb.org/browse/SPEC-947

[estolfo]

Thanks for pointing this out. I chose to retry it because we historically retry read operations in the driver.

(here is where we retry finds, aggregations, and here is where we retry a count, for example)

and I thought it would be inconsistent to not retry the initial aggregation for a change stream. I'll see how the discussion goes in the spec ticket, thanks for opening it.

[ShaneHarvey]

Thanks, this makes a lot more sense now that I know Ruby retries read operations already.

[ShaneHarvey]

Do you need to make a deep copy of the pipeline to ensure the user does not modify it after calling watch?

pipeline = []
change_stream = coll.watch(pipeline, {})
pipeline << {'$project': {_id: 0}}
# Will the next cursor be created with [{'$changeStream': ...}, {'$project': {_id: 0}}]?

Maybe I'm being too paranoid...

[estolfo]

haha, that is possible, I guess. Good call though, it wouldn't hurt to copy it.

[ShaneHarvey]

Similarly, do you need to duplicate the _id to prevent a user from modifying it?

[estolfo]

No, because ObjectIds are immutable and we are caching the ObjectId itself, not the document. If we were caching the document, then we might have to duplicate it. Thanks for checking though.

[ShaneHarvey]

The _id here is a change stream resume token which is a document.

[estolfo]

Oh right, I forgot. hm, I guess I should duplicate it then. Thanks!

[ShaneHarvey]

ChangeStream and #watch LGTM!