-
Notifications
You must be signed in to change notification settings - Fork 640
fix(storage): replace imms with merged imms in any position of staging #14756
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a UT to cover #14743 (maybe via pushing events to event handler)
Unit test added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the quick fix.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Fix #14743
Fix #14724
In this PR, in the
HummockReadVersion
, we will not distinguish betweenimms
andmerged_imms
. All imms will be in a single queue. AMergedImm
version update will not require that the input imms are a suffix of the staging imm list, but only requires that they are consecutive in the staging imm list, and when applying the version update, such consecutive range of list will be replaced by output merged imm.Some sanity check and logs are added by the way.
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.