Update docs for cluster API #30468

PnPie · 2018-05-08T21:21:40Z

What about this for node filters update ?

Relates to #30455

Add docs for some missing node filters.

elasticmachine · 2018-05-08T21:55:44Z

Pinging @elastic/es-core-infra

nik9000 · 2018-05-08T21:59:11Z

docs/reference/cluster.asciidoc

-the APIs either using their internal node id, the node name, address,
-custom attributes, or just the `_local` node receiving the request. For
-example, here are some sample executions of nodes info:
+Most cluster level APIs allow to specify nodes (for example, getting


Can you add something to sentence to explain that specifying nodes limits the results to those nodes? At least, I think that is what specifying the node does here, but I'm no expect.

DaveCTurner

More examples is good, but I think we need a more detailed description of the logic in DiscoveryNodes#resolveNodes() too: it's a comma-separated list of selectors, where each selector is:

_local
_master
_all
a node id or name or IP address or hostname
a pattern (containing * wildcards) that matches node name/IP/hostname
attr:value where attr and value are patterns (containing * wildcards) that match node attributes and values
the filters {master,data,ingest,coordinating_only}:{true,false} which add/remove the selected nodes from the current set.

The selectors run in order: the {master,data,ingest,coordinating_only}:false options means that order is sometimes important.

Also, +1 to @nik9000's comment: I also think it'd be good to describe the effects of specifying a set of nodes in more detail.

DaveCTurner · 2018-05-09T11:40:37Z

docs/reference/cluster.asciidoc

 GET /_nodes/node_name_goes_here
 GET /_nodes/node_name_goes_*
-# Attributes (set something like node.attr.rack: 2 in the config)
+# Using built-in attributes (master, data, ingest or coordinating_only)
+GET /_nodes/master:false,data:false,ingest:true


This is equivalent to the simpler GET /_nodes/ingest:true so I don't think we should have this.

PnPie · 2018-05-10T19:18:28Z

Hi @nik9000 @DaveCTurner
I updated it according to your comments, what do you think of this ?

nik9000

I left a minor thing. @DaveCTurner, what do you think?

nik9000 · 2018-05-10T19:55:31Z

docs/reference/cluster.asciidoc

-custom attributes, or just the `_local` node receiving the request. For
-example, here are some sample executions of nodes info:
+Most cluster level APIs allow to specify nodes with `node filters`, these filters
+limit the results to selected nodes for corresponding APIs.(For example, you can


Could you add a space after the . and before the (?

DaveCTurner

I left a few more comments. I think we need to be a bit more precise in some places still, and there's some formatting nits.

More generally I'm wondering if this would be better-off moving to the common options section, with corresponding updates to the links.

DaveCTurner · 2018-05-11T08:34:51Z

docs/reference/cluster.asciidoc

+Most cluster level APIs allow to specify nodes with `node filters`, these filters
+limit the results to selected nodes for corresponding APIs. (For example, you can
+find it in APIs like `Cluster State`, `Task Management` and all the Nodes API like
+`Nodes States`, `Nodes Info` etc.)


I'm not sure that most cluster-level APIs allow us to specify a set of nodes - by my reckoning it's these:

Cluster Stats (via the (undocumented?) ?nodeId= parameter - could also do with fixing)

Nodes Stats

Nodes Info

Nodes Feature Usage

Task Management API

Nodes hot_threads

There are 13 subheadings in this section, and 6/13 is less than half. (I haven't dug through the code to confirm this so my numbers could be off). In any case it might not be more than half in future, so I think I'd prefer "some" to "most".

Also the things in backticks in this paragraph are not code so I don't think they should be formatted like this. The ones that refer to other pages would be better as links, and node filters is introducing a piece of terminology which we seem either to put in italics or else "in double quotes" depending on taste.

Also also Nodes States should probably be Nodes Stats, and the cluster state API doesn't seem to support a node filter as stated here.

Yes that's right, the Cluster Stats API doesn't support node filters, in fact we can only add /nodes to filter the nodes part of the response but no further filtering for that.

PnPie · 2018-05-11T19:24:37Z

Thanks so much for your review @nik9000 and @DaveCTurner, I address it and for moving to common options section, I'm wondering this node specification part is more specific or only for Cluster level APIs ? and in all the sub-listed apis under Cluster APIs, it is redirected here where the node filters is mentioned. So maybe it's better to keep it here inside Cluster APIs level ?

DaveCTurner · 2018-06-08T08:43:01Z

Hi @PnPie, sorry for the radio silence recently. This is still on my radar.

So maybe it's better to keep it here inside Cluster APIs level ?

I'm inclined to agree. Let's leave it as it is now.

I started out on a review with some suggested wording fixes but it got quite long and I decided it'd be simpler to make the changes on the branch itself. I merged master and pushed 86c08f0 with my suggested changes. Please could you review this and modify as you see fit?

PnPie · 2018-06-08T15:44:59Z

Hi @DaveCTurner
Thanks for the modification and pushing directly, I see the changes you made and it looks (of course) good to me ;)
just for this part: https://github.com/elastic/elasticsearch/pull/30468/files#diff-44f2668cbd91a37d448762ce4e50503eR30, do you think it's better to emphasize it's a custom attr:value and the value could be a pattern (as the 2 previous are also attr:value but pre-defined) ? cause I just feel the 5th one seems quite similar ? but to be honest I'm not good at documentation, so this looks great to me, and if you don't mind you can change it directly in the branch :)

There's been substantial changes since this review. Please could you take another look?

DaveCTurner · 2018-07-02T12:58:28Z

@elasticmachine test this please

DaveCTurner · 2018-07-02T12:58:55Z

@nik9000 could you give this a final check as it changed substantially since your approving review?

javanna · 2018-08-16T13:15:08Z

ping @nik9000 can you check this one please?

DaveCTurner · 2018-08-18T08:59:35Z

Perhaps @ywelsch too/instead? The story here is that @PnPie and I both contributed quite a few changes here so it's improper for me to approve and merge alone.

DaveCTurner · 2018-08-18T09:07:51Z

@elasticmachine test this please

ywelsch

I've left a minor comment (probably just because I got a little confused), looks good otherwise.

ywelsch · 2018-08-20T07:24:05Z

docs/reference/cluster.asciidoc

+* `_local`, to add the local node to the subset.
+* `_master`, to add the currently-elected master node to the subset.
+* a node id, to add this node to the set.
+* a pattern, using `*` wildcards, which adds all nodes to the subset


I find this a little bit confusing. It suggests that name, address, or hostname will only be resolved when using wildcards. Maybe change the previous entry (a node id) to say a node id, node name, address or hostname to add the node with the corresponding id, name, address or hostname to the set to reinforce that exact matching also works.

DaveCTurner · 2018-08-20T07:56:54Z

Good point, thanks. A pattern with no * is still technically a pattern, but the wording is unclear.

@elasticmachine test this please, one last time 🤞

Expands and clarifies exactly what is and isn't allowed when specifying a subset of the nodes as targets of a cluster API, and adds missing links to this from the hot threads and cluster stats API docs. Co-authored-by: David Turner <[email protected]> Co-authored-by: Yu <[email protected]>

* master: Generalize remote license checker (#32971) Trim translog when safe commit advanced (#32967) Fix an inaccuracy in the dynamic templates documentation. (#32890) Logging: Use settings when building daemon threads (#32751) All Translog inner closes should happen after tragedy exception is set (#32674) HLREST: AwaitsFix ML Test Pass DiscoveryNode to initiateChannel (#32958) Add mzn and dz to unsupported locales (#32957) Use settings from the context in BootstrapChecks (#32908) Update docs for node specifications (#30468) HLRC: Forbid all Elasticsearch logging infra (#32784) Only configure publishing if it's applied externally (#32351) Fixes libs:dissect when in eclipse Protect ScriptedMetricIT test cases against failures on 0-doc shards (#32959) (#32968) [Kerberos] Add documentation for Kerberos realm (#32662) Watcher: Properly find next valid date in cron expressions (#32734) Fix some small issues in the getting started docs (#30346) Set forbidden APIs target compatibility to compiler java version (#32935) Move connection listener to ConnectionManager (#32956)

…e-types * elastic/master: (89 commits) Fix assertion in AbstractSimpleTransportTestCase (elastic#32991) [DOC] Splits role mapping APIs into separate pages (elastic#32797) HLRC: ML Close Job (elastic#32943) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) Logging: Use settings when building daemon threads (elastic#32751) All Translog inner closes should happen after tragedy exception is set (elastic#32674) HLREST: AwaitsFix ML Test Pass DiscoveryNode to initiateChannel (elastic#32958) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Only configure publishing if it's applied externally (elastic#32351) Fixes libs:dissect when in eclipse Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) [Kerberos] Add documentation for Kerberos realm (elastic#32662) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) ...

Expands and clarifies exactly what is and isn't allowed when specifying a subset of the nodes as targets of a cluster API, and adds missing links to this from the hot threads and cluster stats API docs. Co-authored-by: David Turner <[email protected]> Co-authored-by: Yu <[email protected]>

* commit '9088d811ce9cff922e6ef1befbeb0f1e0c27016a': (23 commits) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) HLREST: AwaitsFix ML Test Make Geo Context Mapping Parsing More Strict (elastic#32862) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Fix use of deprecated apis Only configure publishing if it's applied externally (elastic#32351) Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) Scripted metric aggregations: add deprecation warning and system (elastic#32944) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) Set forbidden APIs target compatibility to compiler java version (elastic#32935) [TEST] Add "ne" as an unsupported SimpleKdc locale (elastic#32700) MINOR: Remove `IndexTemplateFilter` (elastic#32841) (elastic#32974) INGEST: Create Index Before Pipeline Execute (elastic#32786) (elastic#32975) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764) (elastic#32976) ...

Update docs for cluster API

165eaf8

Add docs for some missing node filters.

nik9000 added >docs General docs changes :Core/Infra/REST API REST infrastructure and utilities labels May 8, 2018

nik9000 requested changes May 8, 2018

View reviewed changes

DaveCTurner requested changes May 9, 2018

View reviewed changes

Update according to comments

5b4c070

nik9000 previously approved these changes May 10, 2018

View reviewed changes

small format change

4344a88

DaveCTurner requested changes May 11, 2018

View reviewed changes

still some changes

1ebb53e

DaveCTurner mentioned this pull request May 22, 2018

GET _nodes/master:false does not work #28797

Closed

DaveCTurner added 2 commits June 8, 2018 08:48

Merge branch 'master' into 2018-06-08-node-filter-docs

ff337e9

Some rewording, and add a note about filters in cluster stats

86c08f0

DaveCTurner added 2 commits June 14, 2018 08:49

Merge branch 'master' into update_cluster_api_docs

4a63d4a

Clarify that both attribute names and values can be patterns

e02ec75

DaveCTurner approved these changes Jun 14, 2018

View reviewed changes

DaveCTurner added 3 commits July 2, 2018 13:56

Merge branch 'master' into update_cluster_api_docs

19c6cb6

Fix regression introduced in merge

dc180f9

Fix regression introduced in merge

779e281

Merge branch 'master' into update_cluster_api_docs

c11a1ba

DaveCTurner added v7.0.0 v6.0.3 v6.1.5 v6.2.5 v6.5.0 v6.3.3 v6.4.1 labels Aug 18, 2018

ywelsch approved these changes Aug 20, 2018

View reviewed changes

Spell out that wildcards are optional

ae3b2ed

DaveCTurner merged commit a883e7d into elastic:master Aug 20, 2018

DaveCTurner removed v6.0.3 v6.1.5 v6.2.5 labels Aug 20, 2018

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

debadair mentioned this pull request Oct 11, 2019

Node filters are undocumented #30455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docs for cluster API #30468

Update docs for cluster API #30468

PnPie commented May 8, 2018

elasticmachine commented May 8, 2018

nik9000 May 8, 2018

DaveCTurner left a comment

DaveCTurner May 9, 2018

PnPie commented May 10, 2018

nik9000 left a comment

nik9000 May 10, 2018

DaveCTurner left a comment

DaveCTurner May 11, 2018

PnPie May 11, 2018 •

edited

Loading

PnPie commented May 11, 2018 •

edited

Loading

DaveCTurner commented Jun 8, 2018

PnPie commented Jun 8, 2018 •

edited

Loading

DaveCTurner commented Jul 2, 2018

DaveCTurner commented Jul 2, 2018

javanna commented Aug 16, 2018

DaveCTurner commented Aug 18, 2018

DaveCTurner commented Aug 18, 2018

ywelsch left a comment

ywelsch Aug 20, 2018

DaveCTurner commented Aug 20, 2018

Update docs for cluster API #30468

Update docs for cluster API #30468

Conversation

PnPie commented May 8, 2018

elasticmachine commented May 8, 2018

nik9000 May 8, 2018

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner May 9, 2018

Choose a reason for hiding this comment

PnPie commented May 10, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 May 10, 2018

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner May 11, 2018

Choose a reason for hiding this comment

PnPie May 11, 2018 • edited Loading

Choose a reason for hiding this comment

PnPie commented May 11, 2018 • edited Loading

DaveCTurner commented Jun 8, 2018

PnPie commented Jun 8, 2018 • edited Loading

DaveCTurner commented Jul 2, 2018

DaveCTurner commented Jul 2, 2018

javanna commented Aug 16, 2018

DaveCTurner commented Aug 18, 2018

DaveCTurner commented Aug 18, 2018

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch Aug 20, 2018

Choose a reason for hiding this comment

DaveCTurner commented Aug 20, 2018

PnPie May 11, 2018 •

edited

Loading

PnPie commented May 11, 2018 •

edited

Loading

PnPie commented Jun 8, 2018 •

edited

Loading