Remove node.max_local_storage_nodes #42428

ywelsch · 2019-05-23T08:16:34Z

This setting, which prior to Elasticsearch 5 was enabled by default and caused all kinds of confusion, has since been disabled by default and is not recommended for production use. The prefered way going forward is for users to explicitly specify separate data folders for each started node to ensure that each node is consistently assigned to the same data path.

A follow-up will move the default data path from ${data.paths}/nodes/0 to ${data.paths}

Relates to #42426

elasticmachine · 2019-05-23T08:16:36Z

Pinging @elastic/es-core-infra

DaveCTurner

So much nicer.

I asked for a bit more tidy-up and spotted some more docs that need adjusting.

DaveCTurner · 2019-05-23T08:38:33Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

-                    MAX_LOCAL_STORAGE_NODES_SETTING.getKey(),
-                    maxLocalStorageNodes);
-                throw new IllegalStateException(message, lastException);
+                    "failed to obtain node locks, tried [%s] with lock id [0];" +


Given that we're changing the tail of this message, we may as well drop the lock id [0] bit too.

fixed in 096e713

DaveCTurner · 2019-05-23T08:42:14Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java

-                Settings.builder().put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), value).build());
-        assertEquals(value, max);
-    }
-
    public void testNodeLockSingleEnvironment() throws IOException {


Don't really need the SingleEnvironment bit of the name any more

fixed in 096e713

DaveCTurner · 2019-05-23T08:44:02Z

.../src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java

            throws Exception {
        final MockTerminal terminal = new MockTerminal();
-        final OptionSet options = command.getParser().parse("-ordinal", Integer.toString(nodeOrdinal));


This option is documented in docs/reference/commands/node-tool.asciidoc, so those docs need adjusting too.

fixed in 68e43e3

DaveCTurner · 2019-05-23T08:57:02Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

+                        return true;
+                    });
+            } catch (LockObtainFailedException e) {
+                // ignore any LockObtainFailedException


I think we can tidy this up a bit more now that there's no retry loop:

no need to capture exception in a local variable, it can be thrown from the catch clause

nodeLock can become final and move inside the outer try.

Do we still need to special-case LockObtainFailedException? Seems ok to me to include the stack trace for all IOExceptions.

fixed in 096e713

DaveCTurner · 2019-05-23T09:00:57Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

+            try {
+                nodeLock = new NodeLock(logger, environment,
+                    dir -> {
+                        Files.createDirectories(dir);


Can we create the directories first, and then this argument can become p -> true? Arguably we could replace it with p -> true everywhere and therefore drop it entirely.

Arguably we could replace it with p -> true everywhere and therefore drop it entirely

not quite. For the node tools, we just select the directories that exist (i.e. possibly a subset). Here, we want to enforce that they all exist.

I think it'd be ok for the node tools to fail if path.data points to paths that don't exist, rather than skipping over them as they do today. You can always adjust path.data to get the tool to run.

I'll mull over this, and open a follow-up PR

DaveCTurner · 2019-05-23T09:01:49Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

            }
            this.locks = nodeLock.locks;
            this.nodePaths = nodeLock.nodePaths;
-            this.nodeLockId = nodeLock.nodeId;
            this.nodeMetaData = loadOrCreateNodeMetaData(settings, logger, nodePaths);

            if (logger.isDebugEnabled()) {


I think this guard is unnecessary.

fixed in 096e713

DaveCTurner

LGTM. I commented about the predicate passed to the constructor of NodeLock but it's ok as-is and/or we can follow up separately with this change.

Allows this setting to be removed in 8.0, see #42428

This setting, which prior to Elasticsearch 5 was enabled by default and caused all kinds of confusion, has since been disabled by default and is not recommended for production use. The preferred way going forward is for users to explicitly specify separate data folders for each started node to ensure that each node is consistently assigned to the same data path. Relates to elastic#42426

The "node.max_local_storage_nodes" property has been removed in 8.0. This commit adds a deprecation info API check for the property. Relates #42404 #42428

Remove node.max-local-storage-nodes

d3ae420

ywelsch added >enhancement >breaking :Core/Infra/Core Core issues without another label v8.0.0 labels May 23, 2019

ywelsch requested a review from DaveCTurner May 23, 2019 08:16

fix test

d7ca11c

DaveCTurner requested changes May 23, 2019

View reviewed changes

ywelsch added 2 commits May 23, 2019 11:05

fix docs

68e43e3

review comments

096e713

ywelsch requested a review from DaveCTurner May 23, 2019 09:30

back to IOException as it was before

d644f42

jasontedor removed the >enhancement label May 23, 2019

DaveCTurner approved these changes May 23, 2019

View reviewed changes

ywelsch added a commit that referenced this pull request May 23, 2019

Deprecate max_local_storage_nodes (#42426)

f57fdc5

Allows this setting to be removed in 8.0, see #42428

ywelsch merged commit c459ea8 into elastic:master May 23, 2019

gwbrown mentioned this pull request May 24, 2019

Update Deprecation Info API checks for 8.0 #42404

Closed

80 tasks

seut mentioned this pull request Aug 28, 2020

Add tutorial on how to remove nodes from a cluster crate/crate-howtos#159

Closed

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

masseyke mentioned this pull request Aug 26, 2021

Feature/deprecation info max local storage nodes #76990

Merged

masseyke added a commit that referenced this pull request Sep 2, 2021

Feature/deprecation info max local storage nodes (#76990)

15ff546

The "node.max_local_storage_nodes" property has been removed in 8.0. This commit adds a deprecation info API check for the property. Relates #42404 #42428

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove node.max_local_storage_nodes #42428

Remove node.max_local_storage_nodes #42428

ywelsch commented May 23, 2019

elasticmachine commented May 23, 2019

DaveCTurner left a comment

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner May 23, 2019

ywelsch May 23, 2019

DaveCTurner left a comment

Remove node.max_local_storage_nodes #42428

Remove node.max_local_storage_nodes #42428

Conversation

ywelsch commented May 23, 2019

elasticmachine commented May 23, 2019

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment