Skip to content

feat(streaming): use single client for exchange #1051

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 18, 2022
Merged

feat(streaming): use single client for exchange #1051

merged 3 commits into from
Mar 18, 2022

Conversation

@skyzh
Copy link
Contributor

@skyzh skyzh commented Mar 18, 2022

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

What's changed and what's your intention?

As title

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 #1023

@skyzh skyzh requested a review from neverchanje March 18, 2022 02:46
@github-actions github-actions bot added the type/feature Type: New feature. label Mar 18, 2022
@skyzh
Copy link
Contributor Author

skyzh commented Mar 18, 2022

Manually checked the log when running ci-3node, only a few ports will be used now.

2022-03-18T02:49:55.871905Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601    
2022-03-18T02:49:55.871867Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601    
2022-03-18T02:49:55.871996Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601    
2022-03-18T02:49:55.872006Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601    
2022-03-18T02:49:55.872003Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601    
2022-03-18T02:49:55.872291Z DEBUG risingwave_compute::rpc::service::exchange_service: Serve stream exchange RPC from 127.0.0.1:57601

@skyzh skyzh mentioned this pull request Mar 18, 2022
Closed
@skyzh skyzh requested a review from BugenZhao March 18, 2022 02:53
@codecov
Copy link

codecov bot commented Mar 18, 2022 •
edited
Loading

Codecov Report

Merging #1051 (ee67be8) into main (b0dbb60) will decrease coverage by 0.04%.
The diff coverage is 48.88%.

@@             Coverage Diff              @@
##               main    #1051      +/-   ##
============================================
- Coverage     71.90%   71.86%   -0.05%     
  Complexity     2766     2766              
============================================
  Files           994      995       +1     
  Lines         83486    83549      +63     
  Branches       1790     1790              
============================================
+ Hits          60034    60039       +5     
- Misses        22561    22619      +58     
  Partials        891      891
Flag Coverage Δ
java 61.01% <ø> (ø)
rust 74.40% <48.88%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
rust/compute/src/rpc/service/exchange_service.rs 0.00% <ø> (ø)
rust/stream/src/task/mod.rs 68.00% <ø> (ø)
rust/stream/src/task/stream_manager.rs 57.16% <18.75%> (-0.52%) ⬇️
rust/stream/src/task/compute_client_pool.rs 33.33% <33.33%> (ø)
rust/stream/src/executor/merge.rs 86.36% <100.00%> (+0.08%) ⬆️
...ust/frontend/src/optimizer/plan_node/batch_sort.rs 63.15% <0.00%> (-34.14%) ⬇️
rust/frontend/src/optimizer/property/order.rs 82.11% <0.00%> (-15.01%) ⬇️
rust/connector/src/filesystem/file_common.rs 80.71% <0.00%> (-0.45%) ⬇️
.../src/executor/managed_state/aggregation/extreme.rs 90.13% <0.00%> (-0.28%) ⬇️

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

BugenZhao
BugenZhao approved these changes Mar 18, 2022
View reviewed changes
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!

@skyzh
better panic message
164383f 
Signed-off-by: Alex Chi <[email protected]>
@skyzh
Copy link
Contributor Author

skyzh commented Mar 18, 2022 •
edited
Loading

Also I made the unwrap message of MergeExecutor more clear. Hope that no one in the future will raise question like Option::unwrap in merge.rs -- the root cause should always be in the upstream executors.

@skyzh skyzh enabled auto-merge (squash) March 18, 2022 03:02
@skyzh
fix clippy errors
ee67be8 
Signed-off-by: Alex Chi <[email protected]>
@skyzh skyzh merged commit 80063ef into main Mar 18, 2022
@skyzh skyzh deleted the skyzh/compute-client-pooling branch March 18, 2022 03:17
neverchanje
neverchanje reviewed Mar 18, 2022
View reviewed changes
rust/stream/src/task/compute_client_pool.rs
impl ComputeClientPool {
pub fn new(cache_capacity: u64) -> Self {
Self {
cache: Cache::new(cache_capacity),
Copy link
Contributor

@neverchanje neverchanje Mar 18, 2022

Choose a reason for hiding this comment

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

We don't have to use Cache actually, we can aggressively store all clients, probably using a hashmap.

Copy link
Contributor Author

@skyzh skyzh Mar 18, 2022

Choose a reason for hiding this comment

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

This cache provides concurrent waiting on a single creation (get_or_insert_with ensures only one client is created for all in-flight get request), where no HashMap could achieve easily... I think we can disable evict on this cache.

Copy link
Contributor

@neverchanje neverchanje Mar 18, 2022

Choose a reason for hiding this comment

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

Ok..., sounds like a tokio::Mutex wrapped hashmap.

@BugenZhao BugenZhao mentioned this pull request Aug 8, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BugenZhao BugenZhao BugenZhao approved these changes

+1 more reviewer

@neverchanje neverchanje neverchanje left review comments

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.

4 participants

@skyzh @neverchanje @BugenZhao