Clean up obsolete code in BlockBasedTable::PrefetchIndexAndFilterBlocks #13277
Conversation
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
PR generally looks good to me. Could you add a release note for this change? I think it's important for users to know in case someone upgrades from a very old version of RocksDB < 7.0.x to the version having this change. https://github.com/facebook/rocksdb/blob/main/unreleased_history/README.txt should have the instruction for the release note. |
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Done! |
There was a problem hiding this comment.
LGTM, but may want to check with @pdillinger in case I am missing any other context for this change.
| @@ -0,0 +1 @@ | |||
| Clean up obsolete fix in `BlockBasedTable::PrefetchIndexAndFilterBlocks` isolated to early 7.0.x releases. | |||
There was a problem hiding this comment.
I believe this will be part of the upcoming major release (10.0). Typically, users pay closer attention to major release upgrades, so it might not be essential to emphasize this. However, it could still be beneficial to highlight that users should be aware of potential performance regressions when upgrading from versions prior to 7.0. They might have filter policy aliases created before 7.0 that will no longer be recognized due to this cleanup.
| @@ -0,0 +1 @@ | |||
| Clean up obsolete fix in `BlockBasedTable::PrefetchIndexAndFilterBlocks` isolated to early 7.0.x releases. | |||
There was a problem hiding this comment.
FWIW, I disagree with release noting this. We know from experience that users have a tendency not to read release notes, so we need to be judicious about what is worthy of inclusion if we expect them to invest their time in reading them. Even if someone has a DB from version 7.0.0 - 7.0.2, it is still fully readable and compactable, just with degraded point read and prefix seek performance, because of lack of filtering.
If we keep a release note, it should talk about user impact of the (obscure case) behavior change.
There was a problem hiding this comment.
Removed the release note.
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mszeszko-meta
force-pushed
the
prefetch_index_and_filter_blocks_cleanup
branch
from
47476fb to
c631394
Compare
|
@mszeszko-meta has updated the pull request. You must reimport the pull request before landing. |
|
@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mszeszko-meta merged this pull request in 44b741e. |
Summary:
As advertised and recommended by original authors comment, we're removing the now-outdated special handling logic for bloom filters perf regression (timing ~release 7.0.X). I decided to keep the
CompatibilityNameas-is since 1) it's publicly exposed API and 2) it's generally useful to have a dedicated name used for identifying whether a filter on disk is readable by the FilterPolicy.Test Plan
'Dead code' / tech debt. As a smoke test, I manually run a similar benchmark to the one in #9736, with ./db_bench built pre and post change.
Generate DB:
Before removing the 'if' block:
After removing the 'if' block: