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): separate inject_barrier_RPC and collect_over_RPC #3129

Merged
merged 10 commits into from
Jun 15, 2022

Conversation

xxhZs
Copy link
Contributor

@xxhZs xxhZs commented Jun 10, 2022

What's changed and what's your intention?

We separate inject_barrier_RPC and barrier_complete_RPC.
Previously, When we inject barrier to CN, the RPC did not return until all collection tasks were complete.
Now , the inject_barrier_RPC can return when we send barrier to all source actor, then barrier_manager will send barrier_complete_RPC to CN wait collection.

In concurrent checkpoint, We need source to send the barrier in order. So we separate inject_barrier_RPC and barrier_complete_RPC. We can inject barrier with single thread to ensure orderly, and concurrently wait for the collection.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

#2184

@xxhZs xxhZs changed the title feat(Meta) Separate inject_barrier_RPC and collect_over_RPC feat(meta): separate inject_barrier_RPC and collect_over_RPC Jun 10, 2022
@xxhZs xxhZs requested a review from hzxa21 June 10, 2022 10:06
proto/meta.proto Outdated Show resolved Hide resolved
proto/meta.proto Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

This implementation seems to make the barrier stuffs much more complicated and harder to read. I'm also wondering if this is really necessary for concurrent checkpoints. Let's have an offline talk on weekdays.

src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/compute/src/rpc/service/stream_service.rs Outdated Show resolved Hide resolved
@BugenZhao BugenZhao requested a review from MrCroxx June 11, 2022 02:54
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #3129 (0177c74) into main (bdac332) will decrease coverage by 0.00%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##             main    #3129      +/-   ##
==========================================
- Coverage   73.59%   73.59%   -0.01%     
==========================================
  Files         744      744              
  Lines      102029   102080      +51     
==========================================
+ Hits        75092    75126      +34     
- Misses      26937    26954      +17     
Flag Coverage Δ
rust 73.59% <53.84%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/compute/src/rpc/service/stream_service.rs 0.00% <0.00%> (ø)
src/meta/src/barrier/progress.rs 17.39% <ø> (ø)
src/meta/src/barrier/recovery.rs 0.00% <ø> (ø)
src/rpc_client/src/meta_client.rs 0.00% <0.00%> (ø)
...c/stream/src/task/barrier_manager/managed_state.rs 78.51% <ø> (ø)
src/stream/src/task/env.rs 0.00% <ø> (ø)
src/stream/src/task/stream_manager.rs 0.00% <0.00%> (ø)
src/stream/src/task/barrier_manager.rs 70.52% <75.00%> (+0.15%) ⬆️
src/meta/src/barrier/mod.rs 69.94% <83.33%> (+0.48%) ⬆️
src/meta/src/stream/stream_manager.rs 69.20% <100.00%> (+0.30%) ⬆️
... and 5 more

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

Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

The code looks cleaner now. Good job.

src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
@xxhZs xxhZs requested a review from BugenZhao June 13, 2022 08:00
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Much clearer now! Generally in the right way.

src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
proto/stream_service.proto Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
@xxhZs xxhZs enabled auto-merge (squash) June 13, 2022 13:21
@xxhZs xxhZs disabled auto-merge June 13, 2022 13:21
@xxhZs xxhZs requested a review from BugenZhao June 14, 2022 02:14
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/meta/src/barrier/mod.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
@xxhZs xxhZs requested a review from BugenZhao June 15, 2022 02:27
@wenym1 wenym1 self-requested a review June 15, 2022 08:04
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM. Great job!

@xxhZs xxhZs merged commit 5b238fb into main Jun 15, 2022
@xxhZs xxhZs deleted the xxh/inject_collect_rpc branch June 15, 2022 08:26
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.

None yet

4 participants