refactor: deprecate max_committed_epoch of hummock version #18644
Conversation
wenym1
changed the base branch from
yiming/no-frontend-max-committed-epoch
to
main
wenym1
changed the base branch from
main
to
yiming/no-frontend-max-committed-epoch
wenym1
force-pushed
the
yiming/deprecate-max-committed-epoch
branch
from
8afc2d5 to
f47530f
Compare
wenym1
added
ci/run-backfill-tests
ci/run-backwards-compat-tests
| .last() | ||
| .map(|level| level.sub_level_id + 1) | ||
| }) | ||
| .unwrap_or(committed_epoch); |
There was a problem hiding this comment.
Can we accept sub-level-id to go backward? Example:
At T1, we create sub-level with id=1
At T2, sub-level id=1 is compacted to base level and all sub-levels are removed
At T3, we create a new sub-level again with id=1
| if let Some(committed_epoch) = committed_epoch { | ||
| for (table_id, info) in version.state_table_info.info() { | ||
| assert_eq!( | ||
| info.committed_epoch, committed_epoch, |
There was a problem hiding this comment.
Just to confirm: the reason why this assertion is valid is because snapshot backfill is non-resumable and tables with fake epochs will be cleaned. In the future with partial ckpt recovery, this assertion will not longer hold.
There was a problem hiding this comment.
Yes, it's only a sanity check for current implementation.
| .last() | ||
| .map(|level| level.sub_level_id + 1) | ||
| }) | ||
| .unwrap_or(committed_epoch); |
There was a problem hiding this comment.
Can we accept sub-level-id to go backward? Example:
At T1, we create sub-level with id=1
At T2, sub-level id=1 is compacted to base level and all sub-levels are removed
At T3, we create a new sub-level again with id=1
…poch' into yiming/deprecate-max-committed-epoch
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
In this PR, the
max_committed_epochis marked as deprecated.The only reason to keep it is only for backward-compatibility when upgrading from a old version that has not filled in the
state_table_infoyet to the current version. During such upgrade, we need to get themax_committed_epochof the latest version, and therefore should still retain the code that maintain the previousmax_committed_epoch.Besides, for an empty cluster with no state table at all, we will handle the periodic barrier and call
commit_epochanyway because we can still bump up themax_committed_epoch. However, after we deprecate themax_committed_epoch,commit_epochon an empty cluster becomes meaningless, because there is no table to bump up thecommitted_epoch. Therefore, in this PR, we change to skip and do not inject a periodic barrier when the cluster is empty. In this way, we can ensure that in eachcommit_epoch, there are some tables to bump up thecommitted_epoch.Moreover, previously we use the
committed_epochas thesub_level_idof the sub level of the newly added sub level. In this PR, we decouple the two concepts, and will use thelatest_sub_level_id + 1as thesub_level_idof newly added sub level.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.