Clarify usage for options ttl and periodic_compaction_seconds for universal compaction
#11552
Conversation
| if (result.periodic_compaction_seconds == kDefaultPeriodicCompSecs && | ||
| is_block_based_table) { | ||
| result.periodic_compaction_seconds = kAdjustedPeriodicCompSecs; | ||
| } |
There was a problem hiding this comment.
This change (enabling regardless of compaction filter in universal) makes sense to me
| if (result.periodic_compaction_seconds == kDefaultPeriodicCompSecs && | ||
| is_block_based_table) { | ||
| result.periodic_compaction_seconds = kAdjustedPeriodicCompSecs; | ||
| } |
There was a problem hiding this comment.
I feel like we should reset result.periodic_compaction_seconds = 0 in case it has a default value and the condition for kAdjustedPeriodicCompSecs is not satisfied (i.e., !is_block_based_table)
There was a problem hiding this comment.
Yes, it's reset to 0 below in lines 427-429, which resets for all compaction styles.
| // Leveled: files older than `periodic_compaction_seconds` will be picked up | ||
| // for compaction and will be re-written to the same level as they were | ||
| // before. |
There was a problem hiding this comment.
It's only bottom-level files, right?
There was a problem hiding this comment.
It's actually for all levels.
include/rocksdb/advanced_options.h
Outdated
| // FIFO: not supported. Setting this option has no effect for FIFO compaction. | ||
| // | ||
| // Universal: when there are files older than `periodic_compaction_seconds`, | ||
| // rocksdb will try to do a full compaction if possible. Such compaction |
There was a problem hiding this comment.
Maybe "full compaction" -> "as large a compaction as possible including the last level"
cbi42
force-pushed
the
universal-prefer-periodic
branch
from
cb7b648
to
e2ddd6f
Compare
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks for the review! Added change log and imported the PR. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
cbi42
force-pushed
the
universal-prefer-periodic
branch
from
f96a1e5
to
5853473
Compare
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: this is stacked on #11550 to further clarify usage of these two options for universal compaction. Similar to FIFO, the two options have the same meaning for universal compaction, which can be confusing to use. For example, for universal compaction, dynamically changing the value of
ttlhas no impact on periodic compactions. Users should dynamically changeperiodic_compaction_secondsinstead. From feature matrix (https://fburl.com/daiquery/5s647hwh), there are instances where users setttlto non-zero value andperiodic_compaction_secondsto 0. For backward compatibility reason, instead of deprecatingttl, comments are added to mention thatperiodic_compaction_secondsare preferred. InSanitizeOptions(), we update the value ofperiodic_compaction_secondsto take into account value ofttl. The logic is documented in relevant option comment.Test plan:
DBTestUniversalCompaction2.PeriodicCompactionDefault