refactor(compaction): deprecate safe_epoch #18491
Conversation
zwang28
force-pushed
the
wangzheng/compaction_ignore_safe_epoch
branch
from
e268a92 to
0e7591f
Compare
zwang28
force-pushed
the
wangzheng/compaction_ignore_safe_epoch
branch
from
0e7591f to
62b477f
Compare
zwang28
changed the title
zwang28
force-pushed
the
wangzheng/compaction_ignore_safe_epoch
branch
from
62b477f to
95d0172
Compare
| @@ -54,7 +54,6 @@ pub struct MetaSnapshotMetadata { | |||
| pub hummock_version_id: HummockVersionId, | |||
| pub ssts: HashSet<HummockSstableObjectId>, | |||
| pub max_committed_epoch: u64, | |||
| pub safe_epoch: u64, | |||
There was a problem hiding this comment.
Will removing this field affect the backward compatibility to the previously created snapshot?
There was a problem hiding this comment.
Ideally for backward compatibiliy, the deserializer should be able to ignore the unknown field safe_epoch. However it doesn't work out of box and requires to further customize MetaSnapshotMetadata's serde. To be specific, the MetaSnapshotMetadata::safe_epoch is ignored automatically, however I don't find a convenient way to make it also ignore the safe_epoch in MetaSnapshotMetadata::state_table_info.
This PR "fixed" the compatibility issue by ignornig the whole state table info from previous meta snapshot. The state table info is used only for query backup, which is rarely used. So I think it's fine.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone)] | ||
| pub struct StateTableInfo { |
There was a problem hiding this comment.
Any reason to reimplement a new struct instead of using the pb?
There was a problem hiding this comment.
It allows more control over how StateTableInfo is serialized. For example, previously a u64 field in PbStateTableInfo will be serialized as String by serde_json, which is not consistent with other fields of MetaSnapshotMetadata. I want to prevent this inconsistency.
zwang28
force-pushed
the
wangzheng/compaction_ignore_safe_epoch
branch
from
bbc1491 to
0d50ccf
Compare
| } else { | ||
| None | ||
| } | ||
| Some(TableWatermarks { |
There was a problem hiding this comment.
In a previous commit 9631102, the changes was reverted. Any reason to change it again?
There was a problem hiding this comment.
Explained here #18491 (comment)
Unlike the previous safe_epoch, relying on earliest_epoch = committed_epoch is too strict and may never be satisfied in my opinion.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
After this PR,
HummockVersion::safe_epoch,HummockVersionDelta::safe_epoch,StateTableInfo::safe_epoch,StateTableInfoDelta::safe_epochhave been removed.safe_epoch.For each vnode, only retain the latest table watermark in HummockVersion.Should merge after #18477
#18214
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.