-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Remove node.max_local_storage_nodes #42428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
[float] | ||
[[breaking_80_node_changes]] | ||
=== Node changes | ||
|
||
//NOTE: The notable-breaking-changes tagged regions are re-used in the | ||
//Installation and Upgrade Guide | ||
//tag::notable-breaking-changes[] | ||
|
||
// end::notable-breaking-changes[] | ||
|
||
[float] | ||
==== Removal of `node.max_local_storage_nodes` setting | ||
|
||
The `node.max_local_storage_nodes` setting was deprecated in 7.x and | ||
has been removed in 8.0. Nodes should be run on separate data paths | ||
to ensure that each node is consistently assigned to the same data path. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,6 @@ | |
import java.util.concurrent.Semaphore; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.function.Predicate; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
@@ -91,9 +90,9 @@ | |
*/ | ||
public final class NodeEnvironment implements Closeable { | ||
public static class NodePath { | ||
/* ${data.paths}/nodes/{node.id} */ | ||
/* ${data.paths}/nodes/0 */ | ||
public final Path path; | ||
/* ${data.paths}/nodes/{node.id}/indices */ | ||
/* ${data.paths}/nodes/0/indices */ | ||
public final Path indicesPath; | ||
/** Cached FileStore from path */ | ||
public final FileStore fileStore; | ||
|
@@ -152,18 +151,11 @@ public String toString() { | |
private final Path sharedDataPath; | ||
private final Lock[] locks; | ||
|
||
private final int nodeLockId; | ||
private final AtomicBoolean closed = new AtomicBoolean(false); | ||
private final Map<ShardId, InternalShardLock> shardLocks = new HashMap<>(); | ||
|
||
private final NodeMetaData nodeMetaData; | ||
|
||
/** | ||
* Maximum number of data nodes that should run in an environment. | ||
*/ | ||
public static final Setting<Integer> MAX_LOCAL_STORAGE_NODES_SETTING = Setting.intSetting("node.max_local_storage_nodes", 1, 1, | ||
Property.NodeScope); | ||
|
||
/** | ||
* Seed for determining a persisted unique uuid of this node. If the node has already a persisted uuid on disk, | ||
* this seed will be ignored and the uuid from disk will be reused. | ||
|
@@ -184,25 +176,23 @@ public String toString() { | |
|
||
public static class NodeLock implements Releasable { | ||
|
||
private final int nodeId; | ||
private final Lock[] locks; | ||
private final NodePath[] nodePaths; | ||
|
||
/** | ||
* Tries to acquire a node lock for a node id, throws {@code IOException} if it is unable to acquire it | ||
* @param pathFunction function to check node path before attempt of acquiring a node lock | ||
*/ | ||
public NodeLock(final int nodeId, final Logger logger, | ||
public NodeLock(final Logger logger, | ||
final Environment environment, | ||
final CheckedFunction<Path, Boolean, IOException> pathFunction) throws IOException { | ||
this.nodeId = nodeId; | ||
nodePaths = new NodePath[environment.dataFiles().length]; | ||
locks = new Lock[nodePaths.length]; | ||
try { | ||
final Path[] dataPaths = environment.dataFiles(); | ||
for (int dirIndex = 0; dirIndex < dataPaths.length; dirIndex++) { | ||
Path dataDir = dataPaths[dirIndex]; | ||
Path dir = resolveNodePath(dataDir, nodeId); | ||
Path dir = resolveNodePath(dataDir); | ||
if (pathFunction.apply(dir) == false) { | ||
continue; | ||
} | ||
|
@@ -248,7 +238,6 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce | |
nodePaths = null; | ||
sharedDataPath = null; | ||
locks = null; | ||
nodeLockId = -1; | ||
nodeMetaData = new NodeMetaData(generateNodeId(settings), Version.CURRENT); | ||
return; | ||
} | ||
|
@@ -257,51 +246,34 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce | |
|
||
try { | ||
sharedDataPath = environment.sharedDataFile(); | ||
IOException lastException = null; | ||
int maxLocalStorageNodes = MAX_LOCAL_STORAGE_NODES_SETTING.get(settings); | ||
IOException exception = null; | ||
|
||
final AtomicReference<IOException> onCreateDirectoriesException = new AtomicReference<>(); | ||
for (int possibleLockId = 0; possibleLockId < maxLocalStorageNodes; possibleLockId++) { | ||
try { | ||
nodeLock = new NodeLock(possibleLockId, logger, environment, | ||
dir -> { | ||
try { | ||
Files.createDirectories(dir); | ||
} catch (IOException e) { | ||
onCreateDirectoriesException.set(e); | ||
throw e; | ||
} | ||
return true; | ||
}); | ||
break; | ||
} catch (LockObtainFailedException e) { | ||
// ignore any LockObtainFailedException | ||
} catch (IOException e) { | ||
if (onCreateDirectoriesException.get() != null) { | ||
throw onCreateDirectoriesException.get(); | ||
} | ||
lastException = e; | ||
} | ||
try { | ||
nodeLock = new NodeLock(logger, environment, | ||
dir -> { | ||
Files.createDirectories(dir); | ||
return true; | ||
}); | ||
} catch (LockObtainFailedException e) { | ||
// ignore any LockObtainFailedException | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can tidy this up a bit more now that there's no retry loop:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 096e713 |
||
} catch (IOException e) { | ||
exception = e; | ||
} | ||
|
||
if (nodeLock == null) { | ||
final String message = String.format( | ||
Locale.ROOT, | ||
"failed to obtain node locks, tried [%s] with lock id%s;" + | ||
" maybe these locations are not writable or multiple nodes were started without increasing [%s] (was [%d])?", | ||
Arrays.toString(environment.dataFiles()), | ||
maxLocalStorageNodes == 1 ? " [0]" : "s [0--" + (maxLocalStorageNodes - 1) + "]", | ||
MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), | ||
maxLocalStorageNodes); | ||
throw new IllegalStateException(message, lastException); | ||
"failed to obtain node locks, tried [%s] with lock id [0];" + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we're changing the tail of this message, we may as well drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 096e713 |
||
" maybe these locations are not writable or multiple nodes were started on the same data path?", | ||
Arrays.toString(environment.dataFiles())); | ||
throw new IllegalStateException(message, exception); | ||
} | ||
this.locks = nodeLock.locks; | ||
this.nodePaths = nodeLock.nodePaths; | ||
this.nodeLockId = nodeLock.nodeId; | ||
this.nodeMetaData = loadOrCreateNodeMetaData(settings, logger, nodePaths); | ||
|
||
if (logger.isDebugEnabled()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this guard is unnecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 096e713 |
||
logger.debug("using node location [{}], local_lock_id [{}]", nodePaths, nodeLockId); | ||
logger.debug("using node location {}", Arrays.toString(nodePaths)); | ||
} | ||
|
||
maybeLogPathDetails(); | ||
|
@@ -334,11 +306,10 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce | |
* Resolve a specific nodes/{node.id} path for the specified path and node lock id. | ||
* | ||
* @param path the path | ||
* @param nodeLockId the node lock id | ||
* @return the resolved path | ||
*/ | ||
public static Path resolveNodePath(final Path path, final int nodeLockId) { | ||
return path.resolve(NODES_FOLDER).resolve(Integer.toString(nodeLockId)); | ||
public static Path resolveNodePath(final Path path) { | ||
return path.resolve(NODES_FOLDER).resolve("0"); | ||
} | ||
|
||
private void maybeLogPathDetails() throws IOException { | ||
|
@@ -805,14 +776,6 @@ public NodePath[] nodePaths() { | |
return nodePaths; | ||
} | ||
|
||
public int getNodeLockId() { | ||
assertEnvIsLocked(); | ||
if (nodePaths == null || locks == null) { | ||
throw new IllegalStateException("node is not configured to store local location"); | ||
} | ||
return nodeLockId; | ||
} | ||
|
||
/** | ||
* Returns all index paths. | ||
*/ | ||
|
@@ -1137,12 +1100,12 @@ private static boolean isIndexMetaDataPath(Path path) { | |
* | ||
* @param indexSettings settings for the index | ||
*/ | ||
public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path sharedDataPath, int nodeLockId) { | ||
public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path sharedDataPath) { | ||
String customDataDir = indexSettings.customDataPath(); | ||
if (customDataDir != null) { | ||
// This assert is because this should be caught by MetaDataCreateIndexService | ||
assert sharedDataPath != null; | ||
return sharedDataPath.resolve(customDataDir).resolve(Integer.toString(nodeLockId)); | ||
return sharedDataPath.resolve(customDataDir).resolve("0"); | ||
} else { | ||
throw new IllegalArgumentException("no custom " + IndexMetaData.SETTING_DATA_PATH + " setting available"); | ||
} | ||
|
@@ -1156,11 +1119,11 @@ public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path s | |
* @param indexSettings settings for the index | ||
*/ | ||
private Path resolveIndexCustomLocation(IndexSettings indexSettings) { | ||
return resolveIndexCustomLocation(indexSettings, sharedDataPath, nodeLockId); | ||
return resolveIndexCustomLocation(indexSettings, sharedDataPath); | ||
} | ||
|
||
private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path sharedDataPath, int nodeLockId) { | ||
return resolveBaseCustomLocation(indexSettings, sharedDataPath, nodeLockId).resolve(indexSettings.getUUID()); | ||
private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path sharedDataPath) { | ||
return resolveBaseCustomLocation(indexSettings, sharedDataPath).resolve(indexSettings.getUUID()); | ||
} | ||
|
||
/** | ||
|
@@ -1172,11 +1135,11 @@ private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path | |
* @param shardId shard to resolve the path to | ||
*/ | ||
public Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId) { | ||
return resolveCustomLocation(indexSettings, shardId, sharedDataPath, nodeLockId); | ||
return resolveCustomLocation(indexSettings, shardId, sharedDataPath); | ||
} | ||
|
||
public static Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId, Path sharedDataPath, int nodeLockId) { | ||
return resolveIndexCustomLocation(indexSettings, sharedDataPath, nodeLockId).resolve(Integer.toString(shardId.id())); | ||
public static Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId, Path sharedDataPath) { | ||
return resolveIndexCustomLocation(indexSettings, sharedDataPath).resolve(Integer.toString(shardId.id())); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create the directories first, and then this argument can become
p -> true
? Arguably we could replace it withp -> true
everywhere and therefore drop it entirely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 adjustpath.data
to get the tool to run.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mull over this, and open a follow-up PR