Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent order being lost for _nodes API filters #42045

Merged
merged 3 commits into from
May 10, 2019

Conversation

astefan
Copy link
Contributor

@astefan astefan commented May 9, 2019

This PR fixes a bug that made the ordering of nodeIds values to be lost. The order in which the node filters are specified is important as it dictates which nodes are being kept and which not in the list for which the metrics are retrieved.

Fixes #41885.

@astefan astefan added the :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. label May 10, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left one comment, looking good otherwise

boolean isMetricsOnly = ALLOWED_METRICS.containsAll(metricsOrNodeIds);
if (isMetricsOnly) {
nodeIds = new String[]{"_all"};
metrics = metricsOrNodeIds;
} else {
nodeIds = metricsOrNodeIds.toArray(new String[]{});
nodeIds = Strings.splitStringByCommaToArray(nodeId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitStringByCommaToArray does not trim whitespace I think whereas tokenizeByCommaToSet did . If we don't want to change the parsing, we should use tokenizeToStringArray here I think.

@astefan
Copy link
Contributor Author

astefan commented May 10, 2019

@ywelsch thanks. I made the change, but also in a second place where the same parsing method was used.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@astefan astefan merged commit 74a7438 into elastic:master May 10, 2019
@astefan astefan deleted the 41885_fix branch May 10, 2019 16:38
astefan added a commit to astefan/elasticsearch that referenced this pull request May 10, 2019
* Switch to using a list instead of a Set for the filters, so that the
order of these filters is kept.

(cherry picked from commit 74a7438)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 10, 2019
* elastic/master: (84 commits)
  [ML] adds geo_centroid aggregation support to data frames (elastic#42088)
  Add documentation for calendar/fixed intervals (elastic#41919)
  Remove global checkpoint assertion in peer recovery (elastic#41987)
  Don't create tempdir for cli scripts (elastic#41913)
  Fix debian-8 update (elastic#42056)
  Cleanup plugin bin directories (elastic#41907)
  Prevent order being lost for _nodes API filters (elastic#42045)
  Change IndexAnalyzers default analyzer access (elastic#42011)
  Remove reference to fs.data.spins in docs
  Mute failing AsyncTwoPhaseIndexerTests
  Remove close method in PageCacheRecycler/Recycler (elastic#41917)
  [ML] adding pivot.max_search_page_size option for setting paging size (elastic#41920)
  Docs: Tweak list formatting
  Simplify handling of keyword field normalizers (elastic#42002)
  [ML] properly nesting objects in document source (elastic#41901)
  Remove extra `ms` from log message (elastic#42068)
  Increase the sample space for random inner hits name generator (elastic#42057)
  Recognise direct buffers in heap size docs (elastic#42070)
  shouldRollGeneration should execute under read lock (elastic#41696)
  Wait for active shard after close in mixed cluster (elastic#42029)
  ...
astefan added a commit that referenced this pull request May 10, 2019
* Switch to using a list instead of a Set for the filters, so that the
order of these filters is kept.

(cherry picked from commit 74a7438)
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
* Switch to using a list instead of a Set for the filters, so that the
order of these filters is kept.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes types filters order not kept for _nodes API
4 participants