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

RUBY-1562 Remove -1 limit in db view #1334

Merged
merged 1 commit into from Mar 29, 2019

Conversation

p-mongo
Copy link
Contributor

@p-mongo p-mongo commented Mar 27, 2019

As far as I can tell the -1 limit is ignored by the driver, just like
a nil limit would be ignored. However the call on server is problematic
with the new retryable reads API since server may change during a
read operation.

@p-mongo p-mongo force-pushed the remove-db-view-limit branch 7 times, most recently from 0cfb2fc to 926103b Compare March 28, 2019 19:42
@p-mongo p-mongo changed the title Remove -1 limit in db view RUBY-1562 Remove -1 limit in db view As far as I can tell the -1 limit is ignored by the driver, ... Mar 28, 2019
@p-mongo p-mongo force-pushed the remove-db-view-limit branch 3 times, most recently from a0bf312 to e037bb7 Compare March 28, 2019 19:45
As far as I can tell the -1 limit is ignored by the driver, just like
a nil limit would be ignored. However the call on server is problematic
with the new retryable reads API since server may change during a
read operation.
@p-mongo p-mongo changed the title RUBY-1562 Remove -1 limit in db view As far as I can tell the -1 limit is ignored by the driver, ... RUBY-1562 Remove -1 limit in db view Mar 28, 2019
@p-mongo p-mongo requested a review from saghm March 29, 2019 16:52
Copy link
Contributor

@saghm saghm left a comment

Choose a reason for hiding this comment

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

The ticket for this is just the retryable reads spec ticket, so I don't have any context for why this change is necessary. Can you explain what the current behavior does that's undesirable?

@p-mongo
Copy link
Contributor Author

p-mongo commented Mar 29, 2019

Existing code changes what command is sent to the server based on the server the command is (going to) be sent to. With retryable reads, the server for a retry may be different than the server for the first attempt, in particular server version may change. Thus the command that was constructed for the first server may not be appropriate for the second server. Luckily, as far as I can tell the limit of -1 is no different from limit being nil, thus this PR removes the limit being set to -1 in some circumstances.

Copy link
Contributor

@saghm saghm left a comment

Choose a reason for hiding this comment

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

Okay, it sounds like the reason this needs to be deleted is the fact that it uses server.features rather than anything to do with the limit itself (which happens to be useless anyhow), which I didn't understand beforehand. Thanks for helping me understand!

@p-mongo p-mongo merged commit f445b78 into mongodb:master Mar 29, 2019
@p-mongo p-mongo deleted the remove-db-view-limit branch March 29, 2019 20:31
p-mongo pushed a commit to p-mongo/mongo-ruby-driver that referenced this pull request Mar 29, 2019
* master:
  RUBY-1562 Remove -1 limit in db view (mongodb#1334)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants