Simplify cluster state management in ESQL #131635
Conversation
idegtiarenko
added
>refactoring
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| * @param indices index expression requested by user | ||
| * @param indicesGrouper grouper of index expressions by cluster alias | ||
| * @param licenseState license state on the querying cluster | ||
| * Checks the index expression for the presence of remote clusters. |
There was a problem hiding this comment.
It looks like this function is the joining of checkForCcsLicense and initializeClusterData - which is fine, but then the comment should reflect that. Right now the comment only talks about checking the license but not the rest of it.
There was a problem hiding this comment.
This is already added below:
| throw EsqlLicenseChecker.invalidLicenseForCcsException(licenseState); | ||
| } | ||
| } | ||
| } catch (NoSuchRemoteClusterException e) { |
There was a problem hiding this comment.
Do I understand it right that NoSuchRemoteClusterException can be thrown by groupIndices? If so, I'd have try/catch to only enclose this call maybe? Otherwise the code flow is a bit un-obvious I think.
There was a problem hiding this comment.
I have tried that initially, however it looked quiet confusing regarding where the flow could terminate.
I would prefer to keep this version.
| assert patterns.size() == 1 : "Only single index pattern is supported"; | ||
| try { | ||
| var groupedIndices = indicesGrouper.groupIndices( | ||
| Set.copyOf(indicesGrouper.getConfiguredClusters()), |
There was a problem hiding this comment.
I think it would be useful to add some comment about why we're copying it here, because I personally don't remember it, and I worked with this code a lot :)
There was a problem hiding this comment.
It is not clear for me either. I copied it from the original place.
It is backed by the mutable data structure below, so this might guard against possible different results when resolving multiple *. Let me add that.
There was a problem hiding this comment.
Oh yes, I think I vaguely remember now that was a concern - since the original map can be changed by settings, we were worried that it could change mid-request and cause all kinds of weirdness. If it's only within groupIndices it's probably less probable, but better to be safe here.
Today we group indices by remote cluster twice. There are also 2 places where remote cluster info is initialized (initializeClusterData or checkForCcsLicense in case of failure).
This change merges them into a single method call as well as ensures
groupIndicesis only called once.