Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Nov 1, 2017

In the docs for 1.7 (doc, src) there was a recommendation for at least 3 master-eligible nodes "in critical clusters" but this was lost when that page was updated in 2.0 (doc, src). I'd like to reinstate this.

@DaveCTurner DaveCTurner requested a review from bleskes November 1, 2017 10:35
@DaveCTurner DaveCTurner added >docs General docs changes v6.1.0 labels Nov 1, 2017

Critical clusters must have at least three master-eligible nodes, with
`minimum_master_nodes` set accordingly, in order to remain available even when
a node fails. A <<rolling-upgrades,rolling upgrade>> also requires at least
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add "which don't allow any downtime" so it's clear what we're aiming at?

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 question.

----------------------------
<1> Defaults to `1`.

Critical clusters must have at least three master-eligible nodes, with
Copy link
Member

Choose a reason for hiding this comment

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

What is a "critical cluster"? What is "must"? We enforce nothing here, I want to understand where this is going, what is the reason for asserting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "critical" terminology was lifted from the 1.7 docs, but perhaps something more descriptive would be better. "Highly-available" perhaps?

"Must" is indeed a little strong - how about "should"?

We don't recommend a redundant set of master-eligible nodes (i.e. ≥ 3) anywhere in the docs as far as I can tell, but I feel that should be written down somewhere.

@DaveCTurner
Copy link
Contributor Author

@jasontedor Is this better?

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 suggestion?

----------------------------
<1> Defaults to `1`.

Clusters should have at least three master-eligible nodes, with
Copy link
Member

Choose a reason for hiding this comment

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

Let's start with why we are doing this: In order to be able to remain available... and finish with what should be done. And maybe "to be able" are needless words?

@DaveCTurner
Copy link
Contributor Author

@jasontedor sure, I've done that.

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.

@DaveCTurner DaveCTurner merged commit fbf8c3e into elastic:master Nov 3, 2017
DaveCTurner added a commit that referenced this pull request Nov 3, 2017
@DaveCTurner
Copy link
Contributor Author

Merged into master and backported to 6.x as b2c7471.

@jasontedor
Copy link
Member

@DaveCTurner I think this can go back to the 5.6 and 6.0 branches as well.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 3, 2017
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 3, 2017
@DaveCTurner
Copy link
Contributor Author

Sure thing, done as 4e07c3a for 5.6 and 154d388 for 6.0.

martijnvg added a commit that referenced this pull request Nov 4, 2017
* es/master:
  Fix snapshot getting stuck in INIT state (#27214)
  Add an example of dynamic field names (#27255)
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix InternalStatsTests
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
martijnvg added a commit that referenced this pull request Nov 4, 2017
* 6.x:
  Add an example of dynamic field names (#27255)
  fixed checkstyle violation
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  [TEST] Fix failing exists query test
  test: Do not run old parent/child tests against a cluster with minimum version greater than 6.0.0
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  fix compil after backport
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
@DaveCTurner DaveCTurner deleted the 2017-11-01-docs-at-least-3-master-nodes branch July 23, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>docs General docs changes v6.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants