-
Notifications
You must be signed in to change notification settings - Fork 460
Draft PR for plugin based approach for custom evaluator for scaling metric evaluation #953
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
base: main
Are you sure you want to change the base?
Conversation
…ration. 2. add sample custom evaluator for simple trend adjustor
*/ | ||
Map<ScalingMetric, EvaluatedScalingMetric> evaluateVertexMetrics( | ||
JobVertexID vertex, | ||
Map<ScalingMetric, EvaluatedScalingMetric> evaluatedMetrics, |
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.
This parameter seems redundant because the context already contains the evaluated Metrics map.
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.
Since we are calling the custom evaluator before adding the current vertex’s evaluated metrics to scaling output. and we have to pass the vertex as an argument, passing evaluatedMetrics separately to the evaluation context sounded good to me!
Let me know if it makes more sense to have the vertex's evaluated metrics as part of the evaluation context itself, will make that update! Thanks!
@@ -382,4 +385,19 @@ private static ConfigOptions.OptionBuilder autoScalerConfig(String key) { | |||
"scaling.key-group.partitions.adjust.mode")) | |||
.withDescription( | |||
"How to adjust the parallelism of Source vertex or upstream shuffle is keyBy"); | |||
|
|||
public static final ConfigOption<String> CUSTOM_EVALUATOR_NAME = |
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.
How about deriving the name from the CustomEvaluator interface? We can load all the names when we load the custom evaluator implementations.
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 so much for your inputs! That Sounds perfect! Will make the update!
…he interface. 2. Updating javadoc’s for the added methods.
…tors for standalone autoscaler entrypoint.
Draft PR for plugin based approach for custom evaluator for scaling metric evaluation
Note: This PR is a prototype implementation for review and feedback.
What is the purpose of the change
(For example: This pull request adds a new feature to periodically create and maintain savepoints through the
FlinkDeployment
custom resource.)Brief change log
(for example:)
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors
: (yes / no)Documentation