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

storage/engine: rename Iterator.{Seek,SeekReverse} to {SeekGE,SeekLT} #42390

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

nvanbenschoten
Copy link
Member

This addresses a comment from #42210.

The commit also adjusts the reverse seek (now SeekLT) to be exclusive instead of
inclusive. This matches the API exposed by Pebble and eliminates the need for
the bug fix in 79b4c77.

This addresses a comment from cockroachdb#42210.

The commit also adjusts the reverse seek (now SeekLT) to be exclusive instead of
inclusive. This matches the API exposed by Pebble and eliminates the need for
the bug fix in 79b4c77.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: thanks for doing this.

The change mostly looks rote. Besides the spanset.Iterator note, was there anything worth special attention here.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @petermattis)

@nvanbenschoten
Copy link
Member Author

Besides the spanset.Iterator note, was there anything worth special attention here.

The only other parts to pay attention to are:

  1. the implementation of rocksDBIterator.SeekLT, which can no longer use r.reseek.
  2. StateLoader.LoadLastIndex, which should still be fine because we're never going to get to Raft log index MaxUint64 (and if we did, we'd have bigger problems) so we can consider it exclusive instead of inclusive.

bors r=petermattis

craig bot pushed a commit that referenced this pull request Nov 12, 2019
42390: storage/engine: rename Iterator.{Seek,SeekReverse} to {SeekGE,SeekLT} r=petermattis a=nvanbenschoten

This addresses a comment from #42210.

The commit also adjusts the reverse seek (now SeekLT) to be exclusive instead of
inclusive. This matches the API exposed by Pebble and eliminates the need for
the bug fix in 79b4c77.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@petermattis
Copy link
Collaborator

StateLoader.LoadLastIndex, which should still be fine because we're never going to get to Raft log index MaxUint64 (and if we did, we'd have bigger problems) so we can consider it exclusive instead of inclusive.

This is the sole use of SeekLT in the code base currently. Your pebbleMVCCScanner refactor will add another one. I'm wondering if we should try to remove SeekLT from the Iterator interface.

@craig
Copy link
Contributor

craig bot commented Nov 12, 2019

Build succeeded

@craig craig bot merged commit 65c42ed into cockroachdb:master Nov 12, 2019
@nvanbenschoten
Copy link
Member Author

This is the sole use of SeekLT in the code base currently. Your pebbleMVCCScanner refactor will add another one. I'm wondering if we should try to remove SeekLT from the Iterator interface.

I don't have strong feelings about this. It's nice to have access to SeekLT and it doesn't weigh down the interface too much, especially because we already have SimpleIterator. I'll defer to your judgment.

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

Successfully merging this pull request may close these issues.

3 participants