Prepare for deprecation of Options::access_hint_on_compaction_start #11658
Conversation
include/rocksdb/options.h
Outdated
| // DEPRECATED | ||
| // Specify the file access pattern once a compaction is started. | ||
| // It will be applied to all input files of a compaction. | ||
| // Default: NORMAL |
There was a problem hiding this comment.
Like #5431, you can add a comment like:
// This flag has no effect on the behavior of compaction and we plan to delete
// it in the future.
Then modify BlockBasedTable::SetupForCompaction() to do nothing.
@ajkr Sorry for the confusion - I should have modified SetupForCompaction() in that PR so that it's clear that (1) file hint does not take effect in compaction read (because of explicit readahead) (2)we don't hint anything in compaction read (modify SetupForCompaction()). My "file hint is not longer needed for compaction read" merely means (1) and I have prematurally closed the issue. Will include "SetupForCompaction()" in this PR and close the issue later. Thanks! |
hx235
force-pushed
the
mark_dep_access_hint_on_compaction_start
branch
from
3b236f8
to
51c5307
Compare
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
We should probably execute this sanitization for rocksdb/db/db_impl/db_impl_open.cc Line 148 in f9de217 |
Good point! |
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
mark_dep_access_hint_on_compaction_start
branch
from
451c01c
to
0815b03
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…_start (facebook#11658)" This reverts commit 09882a5.
Summary: **Context/Summary:** After #11631, we rely on `compaction_readahead_size` for how much to read ahead for compaction read under non-direct IO case. #11658 therefore also sanitized 0 `compaction_readahead_size` to 2MB under non-direct IO, which is consistent with the existing sanitization with direct IO. However, this makes disabling compaction readahead impossible as well as add one more scenario to the inconsistent effects between `Options.compaction_readahead_size=0` during DB open and `SetDBOptions("compaction_readahead_size", "0")` . - `SetDBOptions("compaction_readahead_size", "0")` will disable compaction readahead as its logic never goes through sanitization above while `Options.compaction_readahead_size=0` will go through sanitization. Therefore we decided to do this PR. Pull Request resolved: #11762 Test Plan: Modified existing UTs to cover this PR Reviewed By: ajkr Differential Revision: D48759560 Pulled By: hx235 fbshipit-source-id: b3f85e58bda362a6fa1dc26bd8a87aa0e171af79
Summary: **Context/Summary:** After #11631, we rely on `compaction_readahead_size` for how much to read ahead for compaction read under non-direct IO case. #11658 therefore also sanitized 0 `compaction_readahead_size` to 2MB under non-direct IO, which is consistent with the existing sanitization with direct IO. However, this makes disabling compaction readahead impossible as well as add one more scenario to the inconsistent effects between `Options.compaction_readahead_size=0` during DB open and `SetDBOptions("compaction_readahead_size", "0")` . - `SetDBOptions("compaction_readahead_size", "0")` will disable compaction readahead as its logic never goes through sanitization above while `Options.compaction_readahead_size=0` will go through sanitization. Therefore we decided to do this PR. Pull Request resolved: #11762 Test Plan: Modified existing UTs to cover this PR Reviewed By: ajkr Differential Revision: D48759560 Pulled By: hx235 fbshipit-source-id: b3f85e58bda362a6fa1dc26bd8a87aa0e171af79
In facebook/rocksdb#11658, files are no longer controlled by the access_hint_on_compaction_start flag by removing the content of SetupForCompaction. However, this leads to degradation in compaction read speed when combined with compaction_readahead_size since once a file is opened, in linux, it is hinted with POSIX_FADV_RANDOM. See #787 for full details. Add back the default hint to avoid degradation in this scenario.
In facebook/rocksdb#11658, files are no longer controlled by the access_hint_on_compaction_start flag by removing the content of SetupForCompaction. However, this leads to degradation in compaction read speed when combined with compaction_readahead_size since once a file is opened, in linux, it is hinted with POSIX_FADV_RANDOM. See #787 for full details. Add back the default hint to avoid degradation in this scenario.
Context/Summary:
After #11631, file hint is not longer needed for compaction read. Therefore we can deprecate
Options::access_hint_on_compaction_start. As this is a public API change, we should first mark the relevant APIs (including the Java's) deprecated, remove RocksDB internal call paths where this option is used and then officially remove it in next major release 9.0.Test:
Existing UT