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

fix Seek with lower_bound #3199

Closed

Conversation

zhangjinpeng87
Copy link
Contributor

When Seek a key less than lower_bound, should return lower_bound.
@ajkr PTAL

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Thanks. It'd be good to make the behavior consistent in SeekForPrev if iterator upper bound is set.

Also note the travis failures are related.

db/db_iter.cc Outdated
@@ -1134,6 +1134,13 @@ void DBIter::Seek(const Slice& target) {
saved_key_.Clear();
saved_key_.SetInternalKey(target, sequence_);

if (iterate_upper_bound_ != nullptr &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean iterate_lower_bound_ != nullptr?

@facebook-github-bot
Copy link
Contributor

@zhangjinpeng1987 has updated the pull request. View: changes, changes since last import

@zhangjinpeng87
Copy link
Contributor Author

SeekForPrev will call PrevInternal anyway, so it has handled.

@ajkr
Copy link
Contributor

ajkr commented Nov 28, 2017

From my reading PrevInternal only prevents iterating below lower bound, it doesn't handle the case where we're above upper bound (I guess it is assuming that a valid iterator won't be above upper bound).

@facebook-github-bot
Copy link
Contributor

@zhangjinpeng1987 has updated the pull request. View: changes, changes since last import

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

db/db_iter.cc Outdated
user_comparator_->Compare(saved_key_.GetUserKey(),
*iterate_upper_bound_) >= 0) {
saved_key_.Clear();
saved_key_.SetInternalKey(*iterate_upper_bound_, sequence_);
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry one more thing - should we make the seqnum zero here so the upper bound remains exclusive?

@facebook-github-bot
Copy link
Contributor

@zhangjinpeng1987 has updated the pull request. View: changes, changes since last import

@ajkr
Copy link
Contributor

ajkr commented Nov 29, 2017

you got unlucky :P -- there's a function overload taking bool as argument so passing zero doesn't compile. Anyways we need to do three things:

  1. change the 0 to kMaxSequenceNumber -- I was mistaken earlier and we need to SeekForPrev with a higher seqnum to prevent seeing keys with the same user key.
  2. change the failing assert in DBIteratorPrevNext to expect 1 instead of 7, as now when we SeekToLast with upper bound we don't iterate over the internal keys with same user key as the upper bound,
  3. change SkipStatistics's last assertion to check skip_count increased by 7 instead of 9, as now we don't skip the two internal keys for user key "b" (the upper bound)

@ajkr
Copy link
Contributor

ajkr commented Nov 29, 2017

also be sure to run make -j64 check to verify whether those suggestions worked

@facebook-github-bot
Copy link
Contributor

@zhangjinpeng1987 has updated the pull request. View: changes, changes since last import

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

3 participants