Tweak data node request index handling #118542
Conversation
…csearch into compute-request-no-indices
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
| import static org.elasticsearch.xpack.esql.EsqlTestUtils.loadMapping; | ||
| import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; | ||
|
|
||
| public class DataNodeRequestSerializationTests extends AbstractWireSerializingTestCase<DataNodeRequest> { |
There was a problem hiding this comment.
I renamed the existing DataNodeRequestTests suite to DataNodeRequestSerializationTests, then add a new DataNodeRequestTests. Nothing in DataNodeRequestSerializationTests is new whereas DataNodeRequestTests is net new. Sorry for the confusing diff here.
| @Override | ||
| public IndicesRequest indices(String... indices) { | ||
| this.indices = indices; | ||
| if (Arrays.asList(indices).contains(NO_INDEX_PLACEHOLDER)) { |
There was a problem hiding this comment.
@nik9000 just a heads up: discussed with Jake and its cleaner to move this here since that's what gets called in index resolution with NO_INDEX_PLACEHOLDER (as opposed to keep in the get). LMK if you have objections!
There was a problem hiding this comment.
nit: should we check the length of the String[] for == 1 && contains no index place holder as an optimization ? It could short circuit (or remove) the need to convert to a List as well as a (presumable) O(N) check where N is all the allowed indices per pattern which could be quite a large list for large clusters with many indices.
There was a problem hiding this comment.
good call -- it's actually NO_INDICES_OR_ALIASES_ARRAY that gets passed in which is two elements. So I'm going with Arrays.equals(indices, NO_INDICES_OR_ALIASES_ARRAY) || Arrays.asList(indices).contains(NO_INDEX_PLACEHOLDER) -- Arrays.equals does the optimizations under the hood and then we fall back to the expensive check just to be more future-proof.
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
LGTM
(one minor comment for possible optimization)
n1v0lg
added
the
auto-merge-without-approval
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
Small tweak around how data node requests handle no indices w.r.t. shards. (cherry picked from commit 7585f02)
Small tweak around how data node requests handle no indices w.r.t. shards. (cherry picked from commit 7585f02)
4 participants
Small tweak around how data node requests handle no indices w.r.t. shards.