honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Do not set data paths on no local storage required 

Today when configuring the data paths for the environment, we set data
paths to either the specified path.data or default to data relative to
the Elasticsearch home. Yet if node.local_storage is false, data paths
do not even make sense. In this case, we should reject if path.data is
set, and instead of defaulting data paths to data relative to home, we
should set this to empty paths. This commit does this.

Relates #27587
This commit is contained in:
Jason Tedor 2017-11-29 17:35:00 -05:00 committed by GitHub
parent 29c5540323
commit 55cb8ddd80
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 57 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
core/src/main/java/org/elasticsearch/env/Environment.java vendored
View File

@ -20,6 +20,7 @@
package org.elasticsearch.env;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.io.PathUtils;
import org.elasticsearch.common.settings.Setting;
@ -45,6 +46,9 @@ import java.util.function.Function;
// TODO: move PathUtils to be package-private here instead of
// public+forbidden api!
public class Environment {
private static final Path[] EMPTY_PATH_ARRAY = new Path[0];
public static final Setting<String> PATH_HOME_SETTING = Setting.simpleString("path.home", Property.NodeScope);
public static final Setting<List<String>> PATH_DATA_SETTING =
Setting.listSetting("path.data", Collections.emptyList(), Function.identity(), Property.NodeScope);
@ -103,16 +107,25 @@ public class Environment {
List<String> dataPaths = PATH_DATA_SETTING.get(settings);
final ClusterName clusterName = ClusterName.CLUSTER_NAME_SETTING.get(settings);
if (dataPaths.isEmpty() == false) {
dataFiles = new Path[dataPaths.size()];
dataWithClusterFiles = new Path[dataPaths.size()];
for (int i = 0; i < dataPaths.size(); i++) {
dataFiles[i] = PathUtils.get(dataPaths.get(i));
dataWithClusterFiles[i] = dataFiles[i].resolve(clusterName.value());
if (DiscoveryNode.nodeRequiresLocalStorage(settings)) {
if (dataPaths.isEmpty() == false) {
dataFiles = new Path[dataPaths.size()];
dataWithClusterFiles = new Path[dataPaths.size()];
for (int i = 0; i < dataPaths.size(); i++) {
dataFiles[i] = PathUtils.get(dataPaths.get(i));
dataWithClusterFiles[i] = dataFiles[i].resolve(clusterName.value());
}
} else {
dataFiles = new Path[]{homeFile.resolve("data")};
dataWithClusterFiles = new Path[]{homeFile.resolve("data").resolve(clusterName.value())};
}
} else {
dataFiles = new Path[]{homeFile.resolve("data")};
dataWithClusterFiles = new Path[]{homeFile.resolve("data").resolve(clusterName.value())};
if (dataPaths.isEmpty()) {
dataFiles = dataWithClusterFiles = EMPTY_PATH_ARRAY;
} else {
final String paths = String.join(",", dataPaths);
throw new IllegalStateException("node does not require local storage yet path.data is set to [" + paths + "]");
}
}
if (PATH_SHARED_DATA_SETTING.exists(settings)) {
sharedDataFile = PathUtils.get(PATH_SHARED_DATA_SETTING.get(settings)).normalize();
@ -120,13 +133,13 @@ public class Environment {
sharedDataFile = null;
}
List<String> repoPaths = PATH_REPO_SETTING.get(settings);
if (repoPaths.isEmpty() == false) {
if (repoPaths.isEmpty()) {
repoFiles = EMPTY_PATH_ARRAY;
} else {
repoFiles = new Path[repoPaths.size()];
for (int i = 0; i < repoPaths.size(); i++) {
repoFiles[i] = PathUtils.get(repoPaths.get(i));
}
} else {
repoFiles = new Path[0];
}
// this is trappy, Setting#get(Settings) will get a fallback setting yet return false for Settings#exists(Settings)

31
core/src/test/java/org/elasticsearch/env/EnvironmentTests.java vendored
View File

@ -28,7 +28,10 @@ import java.nio.file.Path;
import static org.hamcrest.CoreMatchers.endsWith;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasToString;
/**
* Simple unit-tests for Environment.java
@ -115,4 +118,32 @@ public class EnvironmentTests extends ESTestCase {
assertThat(environment.configFile(), equalTo(pathHome.resolve("config")));
}
public void testNodeDoesNotRequireLocalStorage() {
final Path pathHome = createTempDir().toAbsolutePath();
final Settings settings =
Settings.builder()
.put("path.home", pathHome)
.put("node.local_storage", false)
.put("node.master", false)
.put("node.data", false)
.build();
final Environment environment = new Environment(settings, null);
assertThat(environment.dataFiles(), arrayWithSize(0));
}
public void testNodeDoesNotRequireLocalStorageButHasPathData() {
final Path pathHome = createTempDir().toAbsolutePath();
final Path pathData = pathHome.resolve("data");
final Settings settings =
Settings.builder()
.put("path.home", pathHome)
.put("path.data", pathData)
.put("node.local_storage", false)
.put("node.master", false)
.put("node.data", false)
.build();
final IllegalStateException e = expectThrows(IllegalStateException.class, () -> new Environment(settings, null));
assertThat(e, hasToString(containsString("node does not require local storage yet path.data is set to [" + pathData + "]")));
}
}

4
core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java vendored
View File

@ -397,14 +397,14 @@ public class NodeEnvironmentTests extends ESTestCase {
}
public void testPersistentNodeId() throws IOException {
String[] paths = tmpPaths();
NodeEnvironment env = newNodeEnvironment(paths, Settings.builder()
NodeEnvironment env = newNodeEnvironment(new String[0], Settings.builder()
.put("node.local_storage", false)
.put("node.master", false)
.put("node.data", false)
.build());
String nodeID = env.nodeId();
env.close();
final String[] paths = tmpPaths();
env = newNodeEnvironment(paths, Settings.EMPTY);
assertThat("previous node didn't have local storage enabled, id should change", env.nodeId(), not(equalTo(nodeID)));
nodeID = env.nodeId();