-
Notifications
You must be signed in to change notification settings - Fork 613
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): use an in-memory meta node to do the replay process locally #6041
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6041 +/- ##
==========================================
+ Coverage 74.70% 74.79% +0.08%
==========================================
Files 933 931 -2
Lines 149025 148812 -213
==========================================
- Hits 111330 111297 -33
+ Misses 37695 37515 -180
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
After I merge main branch, the tool will fail to fetch rows from iter after compaction (if I don't trigger compactions it can read rows). And the test tool on the main branch can reproduce the same problem. I suspect that it is related to the recent refactoring of the hummock (my branch doesn't switch to the new hummock #5902). Could you take a look? @hzxa21 @wenym1 We can further discuss the problem on Slack.
|
a0024e5
to
5d12c0a
Compare
1900c40
to
7014c17
Compare
The |
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
@@ -81,6 +81,9 @@ pub struct MetaOpts { | |||
/// Whether run in compaction detection test mode | |||
pub compaction_deterministic_test: bool, | |||
|
|||
/// Start id of SST table file. See [`IdGeneratorManager`] for details. | |||
pub sst_id_start: Option<u64>, |
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.
For a cluster, this option is actually used only once when initialized first time and is ignored later. Can we avoid this option?
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 need to shift the start sst_id of the embedded Meta node to avoid id conflicts with the original Meta. Any alternatives?
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.
Call get_new_sst_ids
to explicitly fetch then throw away certain sst ids?
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.
Hmm.. I don't get it. Do you mean we should add this logic to the compactor when runs in the deterministic mode?
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.
Discussed offline, will fix it in the next PR.
…an/compaction-test-enhance
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR makes the compaction test tool decouples from the Meta in the cluster being tested, to make the tool self-contained.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.
Types of user-facing changes
Please keep the types that apply to your changes, and remove those that do not apply.
Release note
Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.
Refer to a related PR or issue link (optional)
#5464