Function as though max_local_storage_nodes defaults to 1 in prod mode#19748
Closed
nik9000 wants to merge 2 commits intoelastic:masterfrom
Closed
Function as though max_local_storage_nodes defaults to 1 in prod mode#19748nik9000 wants to merge 2 commits intoelastic:masterfrom
nik9000 wants to merge 2 commits intoelastic:masterfrom
Conversation
Defaulting `max_local_storage_nodes` to `50` is useful for testing so this patch moves that default from "all the time" to "if you don't bind a public ip". If you *do* bind a public IP and attempt to use more than a single node per data directory without *explicitly* setting `max_local_storage_nodes` then Elasticsearch will now fail to start. Since you can't change the default for a setting depending on the "prod-mode/non-prod-mode" flag we instead use a sentinel value (`0`) of `max_local_storage_nodes` to mean "user didn't specify, pick it from the "prod-mode/non-prod-mode" flag". Since we don't know if we're in prod mode when we pick the data directory we instead assume that we *aren't* and then check if the directory that we picked was OK when we run the `BootstrapCheck`s. The nice thing about doing it this way is that we warn the user if they have more than one Elasticsearch in their data directory even if they are running even in dev mode! While this is the easiest way to make this change the cost is that, even if Elasticsearch fails to start because it is in production mode and `max_local_storage_nodes` isn't configured it'll still create a directory. This also removes some of the documentation that suggests you override the setting on all production clusters. That is no longer important because Elasticsearch won't start in production mode if you try to start more than one node. Closes elastic#19679
| */ | ||
| public final class NodeEnvironment implements Closeable { | ||
|
|
||
| private final ESLogger logger; |
Member
Author
There was a problem hiding this comment.
I moved these down below the constants. I thought I was going to add more members so I wanted to fix the order. I didn't end up adding more members but I still like having the members in a sensible order.
Member
|
I think this LGTM, but @jasontedor had a comment about adding the parameter feeling out of place, so maybe wait and see if there is further discussion? |
Member
|
I have objections to the approach here:
I will open a new issue for discussion that relates to this. |
Member
Author
|
Closing in favor of just defaulting the parameter to 1. That is much less hacky. @jasontedor will open the PR in a bit. |
Member
|
I opened #19964. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defaulting
max_local_storage_nodesto50is useful for testing sothis patch moves that default from "all the time" to "if you don't bind
a public ip". If you do bind a public IP and attempt to use more than
a single node per data directory without explicitly setting
max_local_storage_nodesthen Elasticsearch will now fail to start.Since you can't change the default for a setting depending on the
"prod-mode/non-prod-mode" flag we instead use a sentinel value (
0) ofmax_local_storage_nodesto mean "user didn't specify, pick it fromthe "prod-mode/non-prod-mode" flag".
Since we don't know if we're in prod mode when we pick the data directory
we instead assume that we aren't and then check if the directory that
we picked was OK when we run the
BootstrapChecks. The nice thing aboutdoing it this way is that we warn the user if they have more than one
Elasticsearch in their data directory even if they are running even in dev
mode!
While this is the easiest way to make this change the cost is that, even
if Elasticsearch fails to start because it is in production mode and
max_local_storage_nodesisn't configured it'll still create a directory.This also removes some of the documentation that suggests you override the
setting on all production clusters. That is no longer important because
Elasticsearch won't start in production mode if you try to start more
than one node.
Closes #19679