-
Notifications
You must be signed in to change notification settings - Fork 416
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-32057] Support 1.18 rescale api for applying parallelism overrides #614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gyfora
force-pushed
the
FLINK-32057
branch
6 times, most recently
from
June 14, 2023 09:58
72a05b0
to
df9fee2
Compare
gyfora
force-pushed
the
FLINK-32057
branch
5 times, most recently
from
June 15, 2023 07:31
76520f8
to
3d6cc94
Compare
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.
Basically look good, just some minors found.
...rg/apache/flink/kubernetes/operator/observer/deployment/AbstractFlinkDeploymentObserver.java
Show resolved
Hide resolved
...-operator/src/main/java/org/apache/flink/kubernetes/operator/service/NativeFlinkService.java
Show resolved
Hide resolved
...ernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/TestingFlinkService.java
Outdated
Show resolved
Hide resolved
...t/java/org/apache/flink/kubernetes/operator/controller/TestingFlinkDeploymentController.java
Show resolved
Hide resolved
gaborgsomogyi
approved these changes
Jun 15, 2023
mxm
reviewed
Jun 20, 2023
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.
Great work! LGTM
...caler/src/test/java/org/apache/flink/kubernetes/operator/autoscaler/ScalingExecutorTest.java
Show resolved
Hide resolved
...c/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java
Outdated
Show resolved
Hide resolved
...utoscaler/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/ScalingExecutor.java
Show resolved
Hide resolved
Addressed your comments @mxm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
The goal here is to use the newly introduced declarative resource management REST API endpoints in Flink 1.18 to execute parallelism override config changes for job vertexes. This allows us to rescale jobs without performing costly full upgrades (restarting all pods and resources). This will make the autoscaler module much more stable and reliable.
The operator already supported something slightly similar for standalone mode and reactive scheduler configuration where on TM replica change we would simply add new replicas.
Brief change log
Add required REST api request/response classes for 1.18
In order to not rely on unreleased Flink 1.18 libraries, we simply copied the request/response classes for the new rest api.
Combined with e2e tests this should be a good approach.
Async scaling logic
The rescale mechanism in Flink 1.18 is asynchronous and might take a non-defined time to complete. To track progress we trigger the scaling through the
FlinkService#scale
method and afterwards update thelastReconciledSpec
and change the ReconciliationState toUPGRADING
.The observer will subsequently detect an in-progress scaling operation and will check the vertex parallelisms using
FlinkService#scalingCompleted
to move aDEPLOYED
reconciliation state.Note: The rescale api logic is currently only implemented for the native mode. In the standalone mode it would not work as efficiently as the user would need to manually add more TM replicas before the scaling can succeed anyways.
Changes to
@SpecDiff
annotationsPreviously some spec fields were marked for SCALE difftype using the annotation, now it was required to separate what is SCALE / UPGRADE in the different deployment types (Standalone/Native). In native mode we do not support replica/global parallelism changes as scale operations (those rely on standalone + reactive mode).
Required autoscaler changes
We have to improve how we track the job update ts because with in-place rescaling the job can restart without the operator observer noticing (and thus not updating the updateTs). To implement this more robustly we use the Max (last state transition timestamps) from the JobDetailsInfo which we query in every metric collection step. This guarantees that we clear metrics on job restarts / upgrades / failures.
New Resource event and event triggering fixes
The PR contains a fix for the event triggering of spec change events. Currently spec change events are not triggered whenever the job is already in an upgrading state. This is incorrect as new spec changes can happen during an upgrade and those would not be visible to the user. The fix ensures that we trigger spec change events for each different spec generation (whenever the spec changes).
For in-place scaling we introduce a new
Scaling
event which signals the chosen scaling behaviour to the user.Verifying this change
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors
: noDocumentation