-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 version-based validation to reindex requests #38504
Conversation
Pinging @elastic/es-distributed |
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 left a comment that I think it's important. Let me know if I missed something.
DocWriteRequest request, ActionRequestValidationException validationException) { | ||
final long version = request.version(); | ||
final VersionType versionType = request.versionType(); | ||
default ActionRequestValidationException validateVersionAndSeqNoBasedCASParams(ActionRequestValidationException 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.
+1 to default. Nicer.
@@ -100,11 +98,9 @@ public ActionRequestValidationException validate() { | |||
if (false == routingIsValid()) { | |||
e = addValidationError("routing must be unset, [keep], [discard] or [=<some new value>]", e); | |||
} | |||
if (destination.versionType() == INTERNAL) { |
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 is stronger than what validateVersionAndSeqNoBasedCASParams
does. It basically says - if you use internal versioning, you shouldn't set a specific version. I think that's good? also, we probably want the same for external versioning (i.e., the version it self can't be set) and something ifSeqNo/ifPrimary term (which I missed).
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.
Note that the next line here says:
if (destination.version() != Versions.MATCH_ANY && destination.version() != Versions.MATCH_DELETED) {
and validateVersionAndSeqNoBasedCASParams
already has
if (versionType == VersionType.INTERNAL && version != Versions.MATCH_ANY && version != Versions.MATCH_DELETED) {
validationException = addValidationError("internal versioning can not be used for optimistic concurrency control. " +
"Please use `if_seq_no` and `if_primary_term` instead", validationException);
}
These look equivalent to me?
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.
and
validateVersionAndSeqNoBasedCASParams
already has
fair enough (those were folded away and I missed them). Also that would have meant this can't go to 6.x 🤷♂️
Unfortunately this is not working out the way I would like it to. Some of the now-shared validation rules don't work for ReindexRequest (e.g. if the user configures version EXTERNAL, but the request does not contain the version number, which is injected later...). I think we'll have to go back to the previous style of copying rules like done in #37900 |
Relates to #37855