-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add percolator field fallback compatibility #137466
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
Conversation
|
@javanna I'm just looking for some validation that I'm either going in the right direction, or completely off course here :) As we discussed, this is to address that fact that if folks have already upgraded from 8.x to 9.x, and have older indices with percolator fields, we need a way to avoid breaking those folks. |
rjernst
left a comment
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.
Could you add a unit test that trips the fallback case?
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
Outdated
Show resolved
Hide resolved
Yes. Wanted to get a some confirmation that this approach was reasonable. |
andreidan
left a comment
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 for working on this Mark.
This has the right sort of shape for what we need. I think it's going in the right direction.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
I've updated the existing |
jdconrad
left a comment
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! Just left one small question.
| assert valueLength > 0; | ||
|
|
||
| TransportVersion transportVersion; | ||
| if (indexVersion.before(IndexVersions.V_8_8_0)) { |
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 assume when we remove all versions prior to V_8_8_0 this if/else just becomes the else in a safe way now since it'll either reload from source or reject if there is no source?
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 IndexVersion so this won't go away until ES 10.
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.
Ah, yes. Oops! :)
Introduce a "fallback" mechanism to read query builders from percolator fields from source, rather than from the binary field value. This may be necessary in certain cases if the documents were originally ingested with a earlier version of Elasticsearch for which transport serialization compatibility has been removed. In this case the queries can still work, albeit less efficiently. When we do this we emit a warning instructing that users reindex to avoid this additional overhead.