Migrate org.elasticsearch.search to TransportVersion #93091
Conversation
Add initial set of javadocs
Add initial set of javadocs
TransportVersion tests
thecoop
force-pushed
the
transportversion-search
branch
from
c1a0df4 to
64aa21a
Compare
| throw new IllegalArgumentException( | ||
| "Versions before 8.7.0 don't support multiple [knn] search clauses and search was sent to [" | ||
| + out.getVersion() | ||
| "Versions before 8070099 don't support multiple [knn] search clauses and search was sent to [" |
There was a problem hiding this comment.
Do we want to keep Version here?
There was a problem hiding this comment.
I don't think we want Version, it wouldn't make sense. However, this exception seems odd. It looks like some validation that should have happened before we are trying to serialize results. Perhaps someone from the Search area can clarify why such validation is happening so late.
There was a problem hiding this comment.
@benwtrent What is the exception here needed for?
There was a problem hiding this comment.
This is the exact same validation that occurs up in SearchSourceBuilder. We cannot allow serializing this to older nodes if there are more than one KNN search results. Similarly, we don't allow search source builder to serialize to older nodes if it has more than one KNN search clause.
I fail to see how this is a problem here, but NOT a problem in the SearchSourceBuilder.
There was a problem hiding this comment.
It's not a problem, it's more a question on the importance of the message containing the actual self managed node version, vs an opaque version for the transport protocol. What I find odd here is this error being thrown as an IllegalArgumentException, which will go back to users as a 400 http response. Normally such errors should be checked long before we are serializing results.
There was a problem hiding this comment.
If this is only a stopgap exception aimed at developers only, then we can leave it as the transport version
There was a problem hiding this comment.
If this is only a stopgap exception aimed at developers only, then we can leave it as the transport version
Non-administrators wouldn't care about this exception, exactly. Administrators or folks debugging failures will care.
There was a problem hiding this comment.
I think a problem of identifying a "code version running" is not only limited to this one exception here. We will have to have some kind of system with a matrix of transport version - commit/onprem version.
Perhaps for this case specifically we change this as per @benwtrent suggestion. "Cannot serialize multiple KNN results to older node with transport version:" or similar (the 'older node' feels strange)
There was a problem hiding this comment.
I've created #93387 to check exception messages now using transport version
There was a problem hiding this comment.
I've updated the exception message
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
@elasticsearchmachine rerun elasticsearch-ci/part-1 |
5 participants
Migrate files in
org.elasticsearch.searchto useTransportVersionAll changes are
s/Version/TransportVersion/, but see comments