-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-35550][runtime] Move rescaling functionality into dedicated class RescaleManager #24909
Conversation
@1996fanrui : This PR and the linked PRs are ready to be reviewed Keep in mind that follow-up PRs are also including the commits of their base PRs because I chained them but Github doesn't support PR chaining in forks that well. |
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 @XComp for the contribution! IIUC, this PR only has refactor with a series of new interface, and doesn't have any logic change, right?
I left some minor comments, please take a look in your free time.
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/AdaptiveScheduler.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/RescaleManager.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/RescaleManager.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.
I went through it again to see the recent changes and it looks good.
Thanks for your reviews, @ztison and @1996fanrui . I addressed your changes and squashed the commits in a separate force-push (see diff). @1996fanrui Yes, this PR is solely about collecting the rescale-related code in the |
Final force-push to rebase to most-recent |
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 @XComp for the update!
LGTM
… the rescaling logic to improve code testability and extensibility Rescaling is a state-specific functionality. Moving all the logic into Executing state allows us to align the resource controlling in Executing state and WaitingForResources state in a future effort.
Removed unused members. |
CI failure due to FLINK-30719 |
@flinkbot run azure |
PR Chain
What is the purpose of the change
The purpose of this PR is the reorganization of responsibilities for rescaling.
Brief change log
Class diagrams
RescaleManager
,RescaleManager.Context
andRescaleManager.Factory
AdaptiveScheduler
only provides the available parallelism (through theSlotManager
). The rescalingControllers moved intoRescaleManager
Executing
is only in charge of state transitition and savepointsRescaleManager
handles the rescale decisionsVerifying this change
The tests were reorganized accordingly. Some additional unit tests are added to improve coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation