Skip to content

Conversation

@lcawl
Copy link
Contributor

@lcawl lcawl commented May 4, 2017

We need to update the node definitions for the new ML functionality

@lcawl lcawl added >docs General docs changes v6.0.0 labels May 4, 2017
@lcawl lcawl requested review from droberts195 and eskibars May 4, 2017 17:20
@sophiec20 sophiec20 requested a review from skearns64 May 4, 2017 17:24
Copy link
Contributor

Choose a reason for hiding this comment

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

The coordinating node section below also needs to be updated to include ML nodes.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a note about distinguishing the case of X-Pack installed versus not installed.

Copy link
Member

Choose a reason for hiding this comment

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

For a node that does not have X-Pack installed, these settings will cause node startup to fail. There needs to be a caveat to these settings so that users that are not using X-Pack do not copy/paste these into their settings and then get confused when startup fails. This comment applies throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a new commit that adds that information in multiple places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to link to the ML info here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also reword the intro sentence? Instead of "Besides that, each node serves one or more
purpose" maybe "In addition, each node functions as one or more of the following node types:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs tweaked to include ML, too. Maybe say "By default, nodes are master-eligible data and ingest nodes. If X-Pack is installed, they are also machine learning nodes by default."

@lcawl
Copy link
Contributor Author

lcawl commented May 4, 2017

Thanks for all the feedback so far. I've implemented the suggested changes.

Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@lcawl
Copy link
Contributor Author

lcawl commented Jun 9, 2017

I've made some changes, but I won't push them yet because they're dependent on #25164

@lcawl
Copy link
Contributor Author

lcawl commented Jun 13, 2017

I've added links from each node example to the "X-Pack node settings" per feedback from @eskibars, but they're propped so they only appear when we're doing the X-Pack-friendly (index-all.asciidoc) build.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one more comment, then I'm good with it.


ifdef::include-xpack[]
NOTE: These settings apply only when {xpack} is not installed. To create a
standalone master-eligible node when {xpack} is installed, see <<modules-node-xpack,{xpack} node settings>>.
Copy link
Member

Choose a reason for hiding this comment

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

I would say "dedicated" instead of "standalone" here, that's consistent with the other sections and consistent with what is already on the page regarding dedicated master-eligible nodes.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@lcawl lcawl merged commit d181761 into elastic:master Jun 13, 2017
@lcawl lcawl deleted the lcawley-nodes branch June 13, 2017 21:03
lcawl added a commit that referenced this pull request Jun 13, 2017
* [DOCS] Add ML node to node.asciidoc

* [DOCS] Clarify ML node in node.asciidoc

* [DOCS] Add X-Pack icon for admonition blocks

* [DOCS] Formatting X-Pack blocks in node.asciidoc

* [DOCS] Add xpack icon images to node.asciidoc

* [DOCS] Add final xpack role attributes

* [DOCS] Remove unnecssary xpackicon image

* [DOCS] Add link to X-Pack node settings

* [DOCS] Fix path to X-Pack repository

* [DOCS] Add links to X-Pack node settings

* [DOCS] Fixed text for links to X-Pack node settings

* [DOCS] Change standalone node to dedicated node
@lcawl lcawl added the v5.6.0 label Jun 13, 2017
lcawl added a commit that referenced this pull request Jun 13, 2017
* [DOCS] Add ML node to node.asciidoc

* [DOCS] Clarify ML node in node.asciidoc

* [DOCS] Add X-Pack icon for admonition blocks

* [DOCS] Formatting X-Pack blocks in node.asciidoc

* [DOCS] Add xpack icon images to node.asciidoc

* [DOCS] Add final xpack role attributes

* [DOCS] Remove unnecssary xpackicon image

* [DOCS] Add link to X-Pack node settings

* [DOCS] Fix path to X-Pack repository

* [DOCS] Add links to X-Pack node settings

* [DOCS] Fixed text for links to X-Pack node settings

* [DOCS] Change standalone node to dedicated node
@lcawl lcawl added the v5.5.0 label Jun 13, 2017
@lcawl
Copy link
Contributor Author

lcawl commented Jun 13, 2017

Verified that changes appear on external web. For example:
https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-node.html

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 14, 2017
* master: (27 commits)
  Refactor TransportShardBulkAction.executeUpdateRequest and add tests
  Make sure range queries are correctly profiled. (elastic#25108)
  Test: allow setting socket timeout for rest client (elastic#25221)
  Migration docs for elastic#25080 (elastic#25218)
  Remove `discovery.type` BWC layer from the EC2/Azure/GCE plugins elastic#25080
  When stopping via systemd only kill the JVM, not its control group (elastic#25195)
  Remove PrefixAnalyzer, because it is no longer used.
  Internal: Remove Strings.cleanPath (elastic#25209)
  Docs: Add note about which secure settings are valid (elastic#25212)
  Indices.rollover/10_basic should refresh to make the doc visible in lucene stats
  Port support for commercial GeoIP2 databases from Logstash. (elastic#24889)
  [DOCS] Add ML node to node.asciidoc (elastic#24495)
  expose simple pattern tokenizers (elastic#25159)
  Test: add setting to change request timeout for rest client (elastic#25201)
  Fix secure repository-hdfs tests on JDK 9
  Add target_field parameter to gsub, join, lowercase, sort, split, trim, uppercase (elastic#24133)
  Add Cross Cluster Search support for scroll searches (elastic#25094)
  Adapt skip version in rest-api-spec/test/indices.rollover/20_max_doc_condition.yml
  Rollover max docs should only count primaries (elastic#24977)
  Add remote cluster infrastructure to fetch discovery nodes. (elastic#25123)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants