-
Notifications
You must be signed in to change notification settings - Fork 476
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
Handle case of sequence number 0 files for compatibility with RocksDB. #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modulo comment that somehow this scenario is happening in Pebble as well. Do you want me to dig into why?
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)
internal/manifest/version.go, line 461 at r1 (raw file):
// in the file key intervals. This file is placed in L0 since it overlaps in the file // key intervals but since it has no overlapping data, it is assigned a sequence number // of 0 in RocksDB. We handle this case for compatibility with RocksDB.
#316 indicates that somehow Pebble is encountering this case as well. I'm not sure how, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to dig into why?
Seems to be a bug with range tombstones, and least in the case I'm looking at.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, and @sumeerbhola)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for looking. Should I hold off on submitting this?
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, and @sumeerbhola)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this looks good to go. Still tracking down what is going on, but the search is now focused on flushable batches.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ajkr, @itsbilal, and @sumeerbhola)
bf88281
to
b08192e
Compare
Fixes #316