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.

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

feat(meta): support manual compaction for specific sst ids from CN #4338

Merged
merged 2 commits into from
Aug 2, 2022

Conversation

soundOfDestiny
Copy link
Contributor

@soundOfDestiny soundOfDestiny commented Aug 1, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

support manual compaction for specific sst ids from CN, so as that CNs can trigger L0 intra compaction which exactly contains SSTs that generated by itself. see also #4072

Checklist

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.

  • Installation and deployment
  • Connector (sources & sinks)
  • SQL commands, functions, and operators
  • RisingWave cluster configuration changes
  • Other (please specify in the release note below)

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)

proto/hummock.proto Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #4338 (065657f) into main (5066609) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main    #4338   +/-   ##
=======================================
  Coverage   74.47%   74.47%           
=======================================
  Files         846      846           
  Lines      123271   123278    +7     
=======================================
+ Hits        91804    91813    +9     
+ Misses      31467    31465    -2     
Flag Coverage Δ
rust 74.47% <85.71%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/meta/src/rpc/service/hummock_service.rs 6.43% <0.00%> (-0.03%) ⬇️
...src/hummock/compaction/manual_compaction_picker.rs 96.18% <100.00%> (+0.06%) ⬆️
src/meta/src/hummock/compaction/mod.rs 80.97% <100.00%> (+0.10%) ⬆️
src/meta/src/barrier/mod.rs 84.39% <0.00%> (+0.17%) ⬆️
src/meta/src/manager/id.rs 95.10% <0.00%> (+0.54%) ⬆️
src/meta/src/model/barrier.rs 90.00% <0.00%> (+3.33%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Little-Wallace
Copy link
Contributor

I do not understand. It means that meta-node does not trigger L0 compaction for CN ?
But for Non-Overlapping sub-level, the data of each CN must be mixed together. How do you distinguish them ?

@Little-Wallace
Copy link
Contributor

And if one compute-node crash down, each state of one CN may be scheduled to several different CN. Could you change the CN id which it assigned ?

@soundOfDestiny
Copy link
Contributor Author

And if one compute-node crash down, each state of one CN may be scheduled to several different CN. Could you change the CN id which it assigned ?

the new CN will trigger those compaction

@soundOfDestiny
Copy link
Contributor Author

And if one compute-node crash down, each state of one CN may be scheduled to several different CN. Could you change the CN id which it assigned ?

we do not assign anything in meta. each CN itself will request meta to trigger compaction

@soundOfDestiny
Copy link
Contributor Author

I do not understand. It means that meta-node does not trigger L0 compaction for CN ?
But for Non-Overlapping sub-level, the data of each CN must be mixed together. How do you distinguish them ?

for Non-Overlapping sub-level, meta will trigger L0 compaction for them

Copy link
Contributor

@Li0k Li0k left a comment

Choose a reason for hiding this comment

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

manual compaction for specific sst ids is LGTM,if needed we can merge this PR first

@mergify mergify bot merged commit 0f1edcc into main Aug 2, 2022
@mergify mergify bot deleted the zl_mnlcpc branch August 2, 2022 12:02
@hzxa21
Copy link
Collaborator

hzxa21 commented Aug 2, 2022

support manual compaction for specific sst ids from CN, so as that CNs can trigger L0 intra compaction which exactly contains SSTs that generated by itself. see also #4072

Manual compaction on specific SST ids is a good-to-have feature in case admin needs to mitigate production issues when our compaction strategy doesn't work. However, whether or not to allow CNs to trigger L0 intra compaction is debatable. IMO, it is better to avoid that for simplicity.

@soundOfDestiny
Copy link
Contributor Author

support manual compaction for specific sst ids from CN, so as that CNs can trigger L0 intra compaction which exactly contains SSTs that generated by itself. see also #4072

Manual compaction on specific SST ids is a good-to-have feature in case admin needs to mitigate production issues when our compaction strategy doesn't work. However, whether or not to allow CNs to trigger L0 intra compaction is debatable. IMO, it is better to avoid that for simplicity.

https://singularity-data.quip.com/1qKlAaCBNeO0/RFC-maintaining-L0-SST-index-in-CN

nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
…isingwavelabs#4338)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants