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

Jump to bottom

feat(storage): wait committed epoch when update vnode bitmap #20492

Merged
merged 1 commit into from
Feb 25, 2025
Merged

feat(storage): wait committed epoch when update vnode bitmap #20492

merged 1 commit into from
Feb 25, 2025

Conversation

wenym1
Copy link
Contributor

@wenym1 wenym1 commented Feb 13, 2025 •
edited
Loading

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Part of #18312

In this PR, on update_vnode_bitmap of LocalStateStore, we will explicitly wait for the previous epoch to be committed, so that after update_vnode_bitmap returns, the local state store is ready to be read, and doesn't depend on correctly calling try_wait_epoch outside of it.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

@wenym1 wenym1 changed the base branch from main to yiming/log-store-state-post-yield-update-vnode February 13, 2025 12:02
@gru-agent Gru Agent
Copy link

gru-agent bot commented Feb 13, 2025 •
edited
Loading

❌ Rebase test pr failed: Error: Pull request #20492 is not created by gru-agent[bot]

@wenym1 wenym1 mentioned this pull request Feb 13, 2025
Merged
8 tasks
@wenym1 Graphite App
Copy link
Contributor Author

wenym1 commented Feb 13, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@wenym1 wenym1 mentioned this pull request Feb 13, 2025
Merged
8 tasks
Base automatically changed from yiming/log-store-state-post-yield-update-vnode to main February 14, 2025 04:59
@gru-agent Gru Agent
Copy link

gru-agent bot commented Feb 14, 2025 •
edited
Loading

❌ Rebase test pr failed: Error: Pull request #20492 is not created by gru-agent[bot]

@wenym1 wenym1 force-pushed the yiming/wait-epoch-when-update-vnode-bitmap branch from ee10be7 to 079b77c Compare February 14, 2025 07:00
@wenym1 wenym1 requested review from hzxa21, Li0k and BugenZhao February 17, 2025 08:19
@wenym1 wenym1 force-pushed the yiming/wait-epoch-when-update-vnode-bitmap branch from e8a7949 to 832be59 Compare February 20, 2025 09:13
@wenym1 wenym1 requested a review from MrCroxx February 24, 2025 03:10
MrCroxx
MrCroxx approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@MrCroxx MrCroxx left a comment

Choose a reason for hiding this comment

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

LGTM

src/storage/src/memory.rs
return;
}
}
yield_now().await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Had the code here ever caused meta CPU busy looping in some corner cases? Should we add an exponential backoff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The in-mem state store is used mostly for test and reference purpose. So no strong requirement for performance.

Here we assume that the all vnodes are properly assigned, so the for loop is expected to finish in short time as long as we yield the future execution here to let other parallelisms to be driven.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. I forgot this is for the memory state store.

src/storage/src/memory.rs
Comment on lines +1046 to +1056
let prev_epoch = epoch.curr;
epoch.prev = prev_epoch;
epoch.curr = next_epoch;
Copy link
Contributor

Choose a reason for hiding this comment

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

The names of the epochs look confusing here. Let's rename it later.

src/storage/src/memory.rs
{
assert!(prev_epoch > prev_latest_sealed_epoch);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this part of logic affect other callers?

@wenym1 wenym1 added this pull request to the merge queue Feb 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2025
@wenym1 wenym1 force-pushed the yiming/wait-epoch-when-update-vnode-bitmap branch from 832be59 to 5bcc36c Compare February 25, 2025 05:12
@wenym1 wenym1 added this pull request to the merge queue Feb 25, 2025
Merged via the queue into main with commit 0ab6a4a Feb 25, 2025
28 of 29 checks passed
@wenym1 wenym1 deleted the yiming/wait-epoch-when-update-vnode-bitmap branch February 25, 2025 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrCroxx MrCroxx MrCroxx approved these changes

@hzxa21 hzxa21 Awaiting requested review from hzxa21

@Li0k Li0k Awaiting requested review from Li0k

@BugenZhao BugenZhao Awaiting requested review from BugenZhao

Assignees
No one assigned
Labels
type/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wenym1 @MrCroxx