-
Notifications
You must be signed in to change notification settings - Fork 366
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
[CELEBORN-1700][FOLLOWUP] Support ShuffleFallbackCount metric for fallback to vanilla Flink built-in shuffle implementation #3012
Conversation
64a123d
to
b1baa54
Compare
…lback to vanilla Flink built-in shuffle implementation
b1baa54
to
7edde16
Compare
Ping @reswqa, @codenohup. |
Hi, @SteNicholas This PR LGTM, but I have a minor question. It seems that the decision to fall back to the built-in shuffle is based on |
@codenohup, The difference of implementation that Flink only supports job granular fallback but Spark supports shuffle granular fallback causes above fallback metric situation. I would like to support job granular fallback metrics in another pull request. |
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.
LGTM overall except a nit.
...mon/src/main/java/org/apache/celeborn/plugin/flink/fallback/ShuffleFallbackPolicyRunner.java
Outdated
Show resolved
Hide resolved
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.
LGTM.
…anilla Flink built-in shuffle implementation
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.
LGTM. Thanks. Merged into main(v0.6.0).
What changes were proposed in this pull request?
Support
ShuffleFallbackCount
metric for fallback to vanilla Flink built-in shuffle implementation.Why are the changes needed?
#2932 has already supported fallback to vanilla Flink built-in shuffle implementation, which is lack of
ShuffleFallbackCount
metric to feedback the situation of fallback.Does this PR introduce any user-facing change?
No.
How was this patch tested?
RemoteShuffleMasterSuiteJ#testRegisterPartitionWithProducerForForceFallbackPolicy