Skip to content

feat(ctl): support list Hummock version #758

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
Merged
merged 3 commits into from
Mar 9, 2022
Merged

feat(ctl): support list Hummock version #758

merged 3 commits into from
Mar 9, 2022

Conversation

@skyzh
Copy link
Contributor

@skyzh skyzh commented Mar 8, 2022 •
edited
Loading

Signed-off-by: Alex Chi [email protected]

What's changed and what's your intention?

cargo run --bin risectl -- list-version

HummockVersion {
    id: 10,
    levels: [
        Level {
            level_type: Overlapping,
            table_ids: [],
        },
        Level {
            level_type: Nonoverlapping,
            table_ids: [],
        },
    ],
    uncommitted_epochs: [],
    max_committed_epoch: 107920334737113088,
}

One thing I'm not sure: currently, we are creating meta client using worker_id 0. Is it okay to do that? cc @zwang28

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

ref #721

@skyzh skyzh requested review from twocode and zwang28 March 8, 2022 10:01
@github-actions github-actions bot added the type/feature Type: New feature. label Mar 8, 2022
@skyzh skyzh enabled auto-merge (squash) March 8, 2022 10:02
@skyzh skyzh requested a review from TennyZhuang March 8, 2022 10:02
@zwang28
Copy link
Contributor

zwang28 commented Mar 8, 2022 •
edited
Loading

Creating meta client using worker_id 0 is OK as long as the RPCs require correct worker_id are not called. They are pin/unpin/add_tables.
Unfortunately this PR calls pin/unpin. We may treat worker_id 0 as a special reserved id in HummockManager, otherwise HummockManager will reject the meta store writes because it think worker_id 0 is not a live worker in cluster.

You can modify the if let Some(context_id) = context_id in HummockManager::commit_trx.

@skyzh
rename to risectl
4af5b9b 
Signed-off-by: Alex Chi <[email protected]>
@skyzh
Copy link
Contributor Author

skyzh commented Mar 8, 2022

Creating meta client using worker_id 0 is OK as long as the RPCs require correct worker_id are not called. They are pin/unpin/add_tables. Unfortunately this PR calls pin/unpin. We may treat worker_id 0 as a special reserved id in HummockManager, otherwise HummockManager will reject the meta store writes because it think worker_id 0 is not a live worker in cluster.

You can modify the if let Some(context_id) = context_id in HummockManager::commit_trx.

I think we could add a new list_version RPC call to HummockManager. I can do that tomorrow.

@codecov
Copy link

codecov bot commented Mar 8, 2022 •
edited
Loading

Codecov Report

Merging #758 (6580f9d) into main (6a59dc3) will increase coverage by 0.12%.
The diff coverage is 2.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #758      +/-   ##
============================================
+ Coverage     72.28%   72.40%   +0.12%     
- Complexity     2758     2759       +1     
============================================
  Files           910      914       +4     
  Lines         52759    52988     +229     
  Branches       1783     1783              
============================================
+ Hits          38136    38367     +231     
+ Misses        13731    13729       -2     
  Partials        892      892
Flag Coverage Δ
java 61.17% <ø> (+<0.01%) ⬆️
rust 77.15% <2.94%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
rust/cmd/src/bin/compute_node.rs 100.00% <ø> (ø)
rust/cmd/src/bin/frontend_node.rs 100.00% <ø> (ø)
rust/cmd/src/bin/meta_node.rs 100.00% <ø> (ø)
rust/ctl/src/cmd_impl/list_version.rs 0.00% <0.00%> (ø)
rust/ctl/src/common/meta_service.rs 0.00% <0.00%> (ø)
rust/ctl/src/lib.rs 0.00% <0.00%> (ø)
rust/storage/src/monitor/state_store_stats.rs 96.72% <0.00%> (-1.62%) ⬇️
rust/utils/logging/src/lib.rs 0.00% <0.00%> (ø)
rust/cmd/src/bin/ctl.rs 100.00% <100.00%> (+50.00%) ⬆️
rust/frontend/src/expr/literal.rs 60.00% <0.00%> (-11.43%) ⬇️
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a59dc3...6580f9d. Read the comment docs.

@skyzh skyzh disabled auto-merge March 8, 2022 10:44
twocode
twocode approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@twocode twocode left a comment

Choose a reason for hiding this comment

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

Cool. LGTM!

@skyzh
register worker
6580f9d 
Signed-off-by: Alex Chi <[email protected]>
@skyzh skyzh requested a review from yezizp2012 March 9, 2022 04:08
@skyzh
Copy link
Contributor Author

skyzh commented Mar 9, 2022

cc @zwang28 @yezizp2012 I've updated the code to register ctl as RiseCtl worker node. PTAL, thanks!

@skyzh skyzh merged commit 7d220e4 into main Mar 9, 2022
@skyzh skyzh deleted the skyzh/ctl-initial branch March 9, 2022 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twocode twocode twocode approved these changes

@yezizp2012 yezizp2012 yezizp2012 approved these changes

@zwang28 zwang28 Awaiting requested review from zwang28

@TennyZhuang TennyZhuang Awaiting requested review from TennyZhuang

+1 more reviewer

@liurenjie1024 liurenjie1024 liurenjie1024 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type/feature Type: New feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@skyzh @zwang28 @liurenjie1024 @twocode @yezizp2012