-
Notifications
You must be signed in to change notification settings - Fork 594
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
refactor(storage): replace PbHummockVersion with new HummockVersion struct #14101
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #14101 +/- ##
==========================================
- Coverage 68.06% 67.97% -0.09%
==========================================
Files 1548 1549 +1
Lines 267474 267639 +165
==========================================
- Hits 182052 181936 -116
- Misses 85422 85703 +281
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Since PbHummockVersionDelta
is only used to persist, is it necessary to replace it with a new struct ?
The initial motivation for this PR is for the meta data change introduced in #14041. In the issue we will change from using a single global |
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.
rest LGTM.
@@ -45,6 +45,7 @@ pub fn trigger_version_stat( | |||
metrics | |||
.max_committed_epoch | |||
.set(current_version.max_committed_epoch as i64); | |||
// TODO: this op may be costly |
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.
Indeed we've seen version of tens of megabytes. Will any fix include in this PR?
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.
I have changed to implement an estimated_encoded_len
by ourselves.
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.
Rest LGTM
} | ||
} | ||
|
||
pub fn estimated_encode_len(&self) -> usize { |
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.
Does the estimate of size need to correspond to all the member fields?
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.
This is just an estimated encode len. For simplicity I only include the encode len of fields with variable length (e.g. list and map).
…nto yiming/hummock-version-struct
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.
Rest LGTM!
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, we introduce a new separate struct for
HummockVersion
,HummockVersionDelta
andHummockVersionCheckpoint
. This is a refactor PR, and currently the fields in these structs are the same as the counterpart pb structs. This is for the meta data change introduced in #14041. The backward compatibility of meta data will be maintained in thefrom_protobuf
method of these structs. In this PR we have limited all usages of their pb structs in rpc and metadata persistence. All other usages will be using the new struct.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.