-
Notifications
You must be signed in to change notification settings - Fork 493
[FLINK-37126] Add Validator for Autoscaler #936
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
92a1be4
to
15982fd
Compare
439229b
to
85675ee
Compare
24edc0d
to
bcbdf4c
Compare
Any update on this? |
Thanks for @gyfora ping, it's been too long I forgot about this PR, I'll keep pushing forward |
bcbdf4c
to
e9eec8f
Compare
@gyfora Could you help review this PR when you have time? Thank you very much ~ |
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.
The changes look good, but I would remove the extra interface added
import static org.apache.flink.autoscaler.config.AutoScalerOptions.UTILIZATION_TARGET; | ||
|
||
/** Default implementation of {@link AutoscalerValidator}. */ | ||
public class DefaultAutoscalerValidator implements AutoscalerValidator { |
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.
Instead of making this an interface with a default implementation, can we simply make this a regular class and just use it directly? AutoscalerValidator
that contains the implementation. If later we want to make it pluggable then we can extract the interface but now this is just boilerplate
7fe848a
to
3c28969
Compare
8391a13
to
4fd3432
Compare
What is the purpose of the change
Add Validator for Autoscler, Facilitates StandaloneAutoscaler to detect configuration anomalies in advance
Brief change log
DefaultAutoscalerValidator
to the Autoscaler moduleVerifying this change
This change added tests and can be verified as follows:
StandaloneAutoscalerValidatorTest
Used to verify parameter verification under standloneAutoscalerDoes this pull request potentially affect one of the following parts:
CustomResourceDescriptors
: (no)Documentation