-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add support for 'ack watch' to the HLRC. #33962
Conversation
Pinging @elastic/es-core-infra |
9fb51b0
to
c4c52ff
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.
LGTM, 1 nit then you can merge :)
|
||
@Override | ||
public Optional<ValidationException> validate() { | ||
ValidationException exception = new ValidationException(); |
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.
We should try to put all the validation we can into the constructors themselves, because this layer of validation, while it does work, does not tell you where the actual error in setting a bad value came from. There are a lot of requireNotNull
in constructors, so lets move any logic possible into therm
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.
👍
Addresses part of #29827.