ES|QL: do not use skip_unavailable in CPS #135419
Conversation
…lable' into esql/cps_no_skip_unavailable
| EsqlExecutionInfo executionInfo, | ||
| ActionListener<LookupResponse> lookupListener | ||
| ) { | ||
| if (ExceptionsHelper.isRemoteUnavailableException(e) && executionInfo.shouldSkipOnFailure(cluster)) { |
There was a problem hiding this comment.
I think org.elasticsearch.xpack.esql.action.EsqlExecutionInfo#skipOnFailurePredicate is initialized with essentially the same:
But that is definitely confusing that we use different things in different palces
There was a problem hiding this comment.
Yeah, now that predicate is the place where we centralize this behavior.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| listener | ||
| ); | ||
| protected void getRemoteConnection(String cluster, EsqlExecutionInfo executionInfo, ActionListener<Transport.Connection> listener) { | ||
| remoteClusterService.maybeEnsureConnectedAndGetConnection(cluster, executionInfo.shouldSkipOnFailure(cluster) == false, listener); |
There was a problem hiding this comment.
Just to ensure - the desired behavior on CPS is that we always want to try to reconnect immediately if the connection is bad, when allow_partial = true, and don't try to reconnect when allow_partial = false?
There was a problem hiding this comment.
It's a bit of a stretch, as allow_partial is not really about connection retries, but on the other hand the previous behavior relied on skip_unavailable, that is a bit closer to the concept of connection but not really about reconnection either.
In conclusion, IMHO this is fine in terms of consistency (also compared to stateful).
| remoteClusterService.isSkipUnavailable(cluster).orElse(true) == false, | ||
| listener | ||
| ); | ||
| protected void getRemoteConnection(String cluster, EsqlExecutionInfo executionInfo, ActionListener<Transport.Connection> listener) { |
There was a problem hiding this comment.
Not saying this is wrong, but here you don't actually need EsqlExecutionInfo, only one flag that describes executionInfo.shouldSkipOnFailure(cluster) == false which is ensureConnected value. So maybe this should be boolean ensureConnected too?
| String cluster, | ||
| EsqlExecutionInfo executionInfo, | ||
| ActionListener<LookupResponse> lookupListener | ||
| ) { |
There was a problem hiding this comment.
Here again (as below) we don't really need neither cluster nor EsqlExecutionInfo but only boolean executionInfo.shouldSkipOnFailure(cluster). Maybe we could simplify it by calculating this boolean above and pass it to the methods that need it? Just a suggestion.
4 participants
In CPS we don't have
skip_unavailable, we'll useallow_partial_resultsinstead