-
Notifications
You must be signed in to change notification settings - Fork 591
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
fix: Fix the panic issue with the parallelism() call #19849
Conversation
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.
Thanks for the fix. BTW maybe we can add a test for diagnose?
src/prost/src/lib.rs
Outdated
@@ -221,13 +221,21 @@ impl stream_plan::MaterializeNode { | |||
|
|||
// Encapsulating the use of parallelism. | |||
impl common::WorkerNode { | |||
pub fn parallelism(&self) -> usize { | |||
pub fn compute_parallelism(&self) -> usize { |
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.
Does this new name look like "computing" the parallelism when it's called? What about cn_parallelism
or cnode_parallelism
?
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.
Indeed, I originally wanted to call it streaming_parallelism
, but I found that serving node is also using that term. Later, I thought about naming it compute_node_parallelism
, but it seems a bit long.
I'll make a change.
…ptional Signed-off-by: Shanicky Chen <peng@risingwave-labs.com>
…nsistency across modules.
31f3808
to
8bc9659
Compare
Signed-off-by: Shanicky Chen <peng@risingwave-labs.com>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
In theory, only compute nodes should have the concept of parallelism, so invoking
parallelism()
for other types of workers is meaningless and should not be done.This pull request renames the original
parallelism
tocompute_parallelism
and adds a newparallelism
return option to prevent potential misuse.Checklist
Documentation
Release note