Add `_coordinating_only` for nodes resolving in nodes API by PnPie · Pull Request #30313 · elastic/elasticsearch

PnPie · 2018-05-01T19:53:28Z

Add a _coordinating_only filter in nodes API to make it easier to get all the coordinating nodes, which are neither master, nor data, nor ingest nodes.

Close #28831

Add a filter `_coordinating_only` in nodes API to make it easier to get all the coordinating nodes, which are neither master, nor data, nor ingest nodes.

DaveCTurner

The name _coordinating_only seems too long to me. Maybe just _coordinating? Other reviewers' opinions welcome.

DaveCTurner · 2018-05-02T08:12:50Z

server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java

                    .map(DiscoveryNode::getId)
                    .toArray(String[]::new);

-        assertThat(


I think it'd be good to keep this assertion here too.

Looks good now, thanks 👍

DaveCTurner · 2018-05-02T08:24:42Z

docs/reference/cluster.asciidoc

 example, getting the node stats for a node). Nodes can be identified in
 the APIs either using their internal node id, the node name, address,
-custom attributes, or just the `_local` node receiving the request. For
+custom attributes, or just the `_local`, `_master` or `_coordinating_only` nodes receiving the request. For


This doesn't make sense - _local refers to the node receiving the request, but _master and _coordinating_only refer to all master and coordinating nodes. I think it might be better to expand this section of the docs with something like a bulleted list, since this sentence is becoming unwieldy.

Hi @DaveCTurner, I just find this part a little bit confusing when we say "nodes to execute on" or "node receiving the request". If I understand well, all the _local, _master, _coordinating_only or node id, custom attributes etc. are just filters right ? When I do a GET /_nodes/_local it returns to me the node info of the local node, if I do a GET /_nodes/_master, it returns to me the node info of master node ? same for other api like nodes stats, nodes hot_threads etc. So if the local node is the master, I get the same result for the above 2 requests right ? What we would explain here is not "the node to which request was forwarded" ?

DaveCTurner · 2018-05-02T08:25:18Z

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java

     * - a node id
     * - a wild card pattern that will be matched against node names
-     * - a "attr:value" pattern, where attr can be a node role (master, data, ingest etc.) in which case the value can be true of false
+     * - a "attr:value" pattern, where attr can be a node role (master, data, ingest etc.) in which case the value can be true or false,


DaveCTurner · 2018-05-02T08:37:25Z

Correction: reading the earlier discussion I see that we settled on coordinating_only as a filter for nodes, i.e. to support coordinating_only:true and coordinating_only:false following the attr:value pattern. I retract my comment about this being too long: all nodes are technically coordinating nodes so coordinating:true shouldn't do anything, whereas coordinating_only:true should filter out all master, data & ingest nodes.

PnPie · 2018-05-02T20:18:43Z

Is it better to change it to the attr:value pattern ? like currently we just add a _coordinating_only option to filter all the coordinating only nodes, because we don't really care the opposite case coordinating_only:false right ? nodes which are either master, or data or ingest nodes ?

elasticmachine · 2018-05-03T02:21:44Z

Pinging @elastic/es-core-infra

DaveCTurner · 2018-05-03T06:48:11Z

Yes, I think this should follow the attr:value pattern. At least conceptually, coordinating_only is a role similar to master, data and ingest, so it seems better to follow that pattern. The special names _local and _master each refer to a specific node, but that's quite different from this case.

I can think of cases where wanting to say coordinating_only:false is useful: for instance, asking for all data nodes in a particular rack: rack:2,coordinating_only:false,master:false,ingest:false.

bleskes · 2018-05-03T12:00:16Z

The name _coordinating_only seems too long to me. Maybe just _coordinating? Other reviewers' opinions welcome.

the problem with _coordinating is that every node is coordinating in the sense that if you send requests to it it will work just fine. Also there is no way to turn it off.

DaveCTurner · 2018-05-03T12:02:37Z

Yep:

I retract my comment about this being too long: all nodes are technically coordinating nodes so coordinating:true shouldn't do anything, whereas coordinating_only:true should filter out all master, data & ingest nodes.

javanna · 2018-05-03T12:16:40Z

maybe coord_only if we feel that coordinating_only is too long? :)

PnPie · 2018-05-06T15:35:46Z

Hi, I update the PR and I see it should follow the attr:value pattern. I also see that we have used the term coordinating_only in cluster stats api, so I leave it as that at this moment, let's see shall we need to make it shorter.

DaveCTurner

Looks good. There's one more code change to make and then it's just docs and the 6.4 changelog to update.

DaveCTurner · 2018-05-08T06:49:48Z

server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java

                    .map(DiscoveryNode::getId)
                    .toArray(String[]::new);

-        assertThat(


Looks good now, thanks 👍

DaveCTurner · 2018-05-08T09:00:37Z

server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java

            Set<String> matchingNodeIds(DiscoveryNodes nodes) {
                return Collections.singleton(nodes.getMasterNodeId());
            }
+        }, COORDINATING_ONLY("_coordinating_only") {


This looks like a hangover of the previous approach - should be coordinating_only:true I think.

PnPie · 2018-05-08T12:18:09Z

Hi @DaveCTurner
I updated the PR and docs, do you know how to update the changelog ? It seems I've never done this before ?

DaveCTurner · 2018-05-08T13:25:49Z

@elasticmachine test this please

@PnPie I updated the changelog as this seemed simpler than describing how to do so :) Hope that's ok. I also merged master in.

jasontedor · 2018-05-08T13:55:28Z

test this please

DaveCTurner · 2018-05-08T14:32:56Z

I'm tempted to suggest reverting the docs change - I never actually checked that the other options are documented, and they aren't (yet -- I opened #30455 for this) so your addition to the docs is not really necessary. WDYT?

PnPie · 2018-05-08T14:58:52Z

Okay, as anyway we are going to update the docs for all there, I can create a PR for that one if needed. So I'll revert it after the CI run.

DaveCTurner · 2018-05-08T16:13:41Z

That'd be awesome, thanks.

…elasticsearch into coordinating_only_nodes_resolving

DaveCTurner

LGTM, thanks @PnPie

DaveCTurner · 2018-05-09T05:23:34Z

@elasticmachine test this please

Today we can execute cluster API actions on only master, data or ingest nodes using the `master:true`, `data:true` and `ingest:true` filters, but it is not so easy to select coordinating-only nodes (i.e. those nodes that are neither master nor data nor ingest nodes). This change fixes this by adding support for a `coordinating_only` filter such that `coordinating_only:true` adds all coordinating-only nodes to the set of selected nodes, and `coordinating_only:false` deletes them. Resolves #28831.

…or-you * elastic/master: (22 commits) Docs: Test examples that recreate lang analyzers (elastic#29535) BulkProcessor to retry based on status code (elastic#29329) Add GET Repository High Level REST API (elastic#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (elastic#30313) Stop forking groovyc (elastic#30471) Avoid setting connection request timeout (elastic#30384) Use date format in `date_range` mapping before fallback to default (elastic#29310) Watcher: Increase HttpClient parallel sent requests (elastic#30130) Mute ML upgrade test (elastic#30458) Stop forking javac (elastic#30462) Client: Deprecate many argument performRequest (elastic#30315) Docs: Use task_id in examples of tasks (elastic#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (elastic#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (elastic#30434) Avoid NPE in `more_like_this` when field has zero tokens (elastic#30365) Build: Switch to building javadoc with html5 (elastic#30440) Add a quick tour of the project to CONTRIBUTING (elastic#30187) Reindex: Use request flavored methods (elastic#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (elastic#30432) ...

* master: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) add version compatibility from 6.4.0 after backport, see #30319 (#30390) Security: Simplify security index listeners (#30466) Add proper longitude validation in geo_polygon_query (#30497) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Solve Gradle deprecation warnings around shadowJar (#30483) SAML: Process only signed data (#30420) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163) Silence IndexUpgradeIT test failures. (#30430) Bump Gradle heap to 1792m (#30484) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) Test: remove hardcoded list of unconfigured ciphers (#30367) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) BulkProcessor to retry based on status code (#29329) Add GET Repository High Level REST API (#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (#30313) Stop forking groovyc (#30471) Avoid setting connection request timeout (#30384) Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) # Conflicts: # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

* 6.x: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) BulkProcessor to retry based on status code (#29329) Avoid setting connection request timeout (#30384) Test: remove hardcoded list of unconfigured ciphers (#30367) Add GET Repository High Level REST API (#30362) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) add a comment explaining the need for RetryOnReplicaException on missing mappings Pass the task to broadcast actions (#29672) Stop forking groovyc (#30471) Add `coordinating_only` node selector (#30313) Fix accidental error in changelog Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) [Security][Tests] Azeri(Turkish) locale tripps opensaml dependency

Add _coordinating_only for nodes resolving in nodes API

d2ef73c

Add a filter `_coordinating_only` in nodes API to make it easier to get all the coordinating nodes, which are neither master, nor data, nor ingest nodes.

DaveCTurner reviewed May 2, 2018

View reviewed changes

colings86 added >enhancement :Core/Infra/Stats Statistics tracking and retrieval APIs labels May 2, 2018

hub-cap added :Core/Infra/Stats Statistics tracking and retrieval APIs and removed :Core/Infra/Stats Statistics tracking and retrieval APIs labels May 3, 2018

PnPie added 2 commits May 6, 2018 14:43

Revert: cluster.asciidoc changes

2b6eac6

Change to attr:value pattern

2c50745

DaveCTurner requested changes May 8, 2018

View reviewed changes

DaveCTurner added v7.0.0 v6.4.0 labels May 8, 2018

change previous modif and update docs

1f51cf7

DaveCTurner added 3 commits May 8, 2018 13:25

Add CHANGELOG entry

7a73b41

Merge branch 'master' into pr/30313

6082969

Move CHANGELOG entry

3f1a3e6

DaveCTurner mentioned this pull request May 8, 2018

Node filters are undocumented #30455

Closed

revert docs changes

aa337fc

Merge branch 'coordinating_only_nodes_resolving' of github.com:PnPie/…

e905aaa

…elasticsearch into coordinating_only_nodes_resolving

DaveCTurner approved these changes May 9, 2018

View reviewed changes

DaveCTurner merged commit 106bed9 into elastic:master May 9, 2018

DaveCTurner mentioned this pull request May 22, 2018

GET _nodes/master:false does not work #28797

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

DaveCTurner mentioned this pull request Dec 16, 2025

Node resolution using coordinating_only node attribute has (apparently) incorrect behaviour #139587

Open

Conversation

PnPie commented May 1, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 2, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 8, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 2, 2018

Choose a reason for hiding this comment

Uh oh!

PnPie May 2, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 2, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented May 2, 2018

Uh oh!

PnPie commented May 2, 2018

Uh oh!

elasticmachine commented May 3, 2018

Uh oh!

DaveCTurner commented May 3, 2018

Uh oh!

bleskes commented May 3, 2018

Uh oh!

DaveCTurner commented May 3, 2018

Uh oh!

javanna commented May 3, 2018

Uh oh!

PnPie commented May 6, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 8, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 8, 2018

Choose a reason for hiding this comment

Uh oh!

PnPie commented May 8, 2018

Uh oh!

DaveCTurner commented May 8, 2018

Uh oh!

jasontedor commented May 8, 2018

Uh oh!

DaveCTurner commented May 8, 2018

Uh oh!

PnPie commented May 8, 2018

Uh oh!

DaveCTurner commented May 8, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DaveCTurner commented May 9, 2018 •

edited

Loading