Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use EpochWithGap::new_max_epoch if the provided epoch is max epoch #13881

Merged
merged 3 commits into from
Dec 10, 2023
Merged

fix: use EpochWithGap::new_max_epoch if the provided epoch is max epoch #13881

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
798c36f
fix: use EPochWithGap::new_max_epoch if the provided epoch is max epoch
hzxa21 Dec 8, 2023
5bf6bfd
fix compile error
hzxa21 Dec 8, 2023
0bdcc88
address comment
hzxa21 Dec 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/common/src/util/epoch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,11 @@ impl Epoch {

pub const EPOCH_AVAILABLE_BITS: u64 = 16;
pub const MAX_SPILL_TIMES: u16 = ((1 << EPOCH_AVAILABLE_BITS) - 1) as u16;
pub const EPOCH_MASK: u64 = (1 << EPOCH_AVAILABLE_BITS) - 1;
pub const MAX_EPOCH: u64 = u64::MAX & !EPOCH_MASK;
// Low EPOCH_AVAILABLE_BITS bits set to 1
pub const EPOCH_SPILL_TIME_MASK: u64 = (1 << EPOCH_AVAILABLE_BITS) - 1;
// High (64-EPOCH_AVAILABLE_BITS) bits set to 1
pub const EPOCH_MASK: u64 = !EPOCH_SPILL_TIME_MASK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EPOCH_MASK is fully flipped compared to before this PR. It is used anywhere else? If not I think we can remove this const or make it private.

pub const MAX_EPOCH: u64 = u64::MAX & EPOCH_MASK;

pub fn is_max_epoch(epoch: u64) -> bool {
// Since we have write `MAX_EPOCH` as max epoch to sstable in some previous version,
Expand Down
17 changes: 9 additions & 8 deletions src/storage/hummock_sdk/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ mod key_cmp;
use std::cmp::Ordering;

pub use key_cmp::*;
use risingwave_common::util::epoch::EPOCH_MASK;
use risingwave_common::util::epoch::EPOCH_SPILL_TIME_MASK;
use risingwave_pb::common::{batch_query_epoch, BatchQueryEpoch};
use risingwave_pb::hummock::SstableInfo;

Expand Down Expand Up @@ -284,11 +284,12 @@ impl EpochWithGap {
// So for compatibility, we must skip checking it for u64::MAX. See bug description in https://github.com/risingwavelabs/risingwave/issues/13717
#[cfg(not(feature = "enable_test_epoch"))]
{
debug_assert!(
((epoch & EPOCH_MASK) == 0) || risingwave_common::util::epoch::is_max_epoch(epoch)
);
let epoch_with_gap = epoch + spill_offset as u64;
EpochWithGap(epoch_with_gap)
if risingwave_common::util::epoch::is_max_epoch(epoch) {
EpochWithGap::new_max_epoch()
} else {
debug_assert!((epoch & EPOCH_SPILL_TIME_MASK) == 0);
EpochWithGap(epoch + spill_offset as u64)
}
}
#[cfg(feature = "enable_test_epoch")]
{
Expand Down Expand Up @@ -322,7 +323,7 @@ impl EpochWithGap {
pub fn pure_epoch(&self) -> HummockEpoch {
#[cfg(not(feature = "enable_test_epoch"))]
{
self.0 & !EPOCH_MASK
self.0 & !EPOCH_SPILL_TIME_MASK
}
#[cfg(feature = "enable_test_epoch")]
{
Expand All @@ -331,6 +332,6 @@ impl EpochWithGap {
}

pub fn offset(&self) -> u64 {
self.0 & EPOCH_MASK
self.0 & EPOCH_SPILL_TIME_MASK
}
}