Refactor CCS handling code from EsqlSession and IndexResolver into dedicated util class #115976
Conversation
quux00
added
>refactoring
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
…sqlSessionCCSUtils
…as testReturnSuccessWithEmptyResult
quux00
force-pushed
the
esql/refactor-ccs-EsqlSession
branch
from
9bb8af3 to
84163d8
Compare
quux00
requested review from
astefan,
costin,
pawankartik-elastic and
smalyshev
| null | ||
| ); | ||
| ActionListener.wrap(plan -> executeOptimizedPlan(request, executionInfo, runPhase, optimizedPlan(plan), listener), e -> { | ||
| if (EsqlSessionCCSUtils.returnSuccessWithEmptyResult(executionInfo, e)) { |
There was a problem hiding this comment.
I wonder if we could go further and put this logic into EsqlSessionCCSUtils too. So we'd have just a method name here. After all, creating an empty result is also a generic function that is not very specific to EsqlSession?
| EsqlExecutionInfo.Cluster cluster = executionInfo.getCluster(clusterAlias); | ||
| if (cluster.getStatus() != EsqlExecutionInfo.Cluster.Status.SKIPPED) { | ||
| if (cluster.getClusterAlias().equals(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY)) { | ||
| sb.append(executionInfo.getCluster(clusterAlias).getIndexExpression()).append(','); |
There was a problem hiding this comment.
executionInfo.getCluster(clusterAlias).getIndexExpression() is called in both the if branches. I wonder if it'd be better to pull it out?
| } | ||
| } | ||
|
|
||
| if (sb.length() > 0) { |
There was a problem hiding this comment.
Suggestion: use isEmpty()?
| for (String clusterAlias : executionInfo.clusterAliases()) { | ||
| if (executionInfo.isSkipUnavailable(clusterAlias) == false | ||
| && clusterAlias.equals(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY) == false) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
What's your opinion on writing this as:
return executionInfo.clusterAliases()
.stream()
.noneMatch(clusterAlias -> executionInfo.isSkipUnavailable(clusterAlias) == false && clusterAlias.equals(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY) == false
);
|
This refactoring overlaps a lot of the refactoring Costin did in his PR around lookups: #115813 (review) so probably makes sense to close this PR and wait for his to merge. |
|
Closing since Costin has done much of this refactoring in #115813. |
Pure refactoring PR
Pure refactoring PR
Pure refactoring PR
4 participants
This a pure refactoring ticket. No new functionality was added.
A few new tests were added since they are exposed in a place for easier testing.
Based on #115266 (review)