Add `path.shared_data` #12729

dakrone · 2015-08-07T15:43:57Z

This allows path.shared_data to be added to the security manager while
still allowing a custom data_path for indices using shadow replicas.

For example, configuring path.shared_data: /tmp/foo, then created an
index with:

POST /myindex
{
  "index": {
    "number_of_shards": 1,
    "number_of_replicas": 1,
    "data_path": "/tmp/foo/bar/baz",
    "shadow_replicas": true
  }
}

The index will then reside in /tmp/foo/bar/baz.

path.shared_data defaults to ${path.home}/data if not specified.

Resolves #12714
Relates to #11065

jpountz · 2015-08-07T16:03:55Z

core/src/test/java/org/elasticsearch/test/ESSingleNodeTestCase.java

-            .put(InternalSettingsPreparer.IGNORE_SYSTEM_PROPERTIES_SETTING, true) // make sure we get what we set :)
+                .put(ClusterName.SETTING, InternalTestCluster.clusterName("single-node-cluster", randomLong()))
+                .put("path.home", createTempDir())
+                .put("path.shared_data", createTempDir().getParent())


why the .getParent?

I wanted it to be a level higher than the temp directory, to capture the directory that other temp dirs are created in.

Why not create one temp dir, and then a subdir off of that? There is nothing that guarantees tempdirs are all created side by side.

Because the directory set for path.shared_data and the custom directories set during tests when indices are created are uncoupled, if I set it to a specific directory in advance, every test (including where we randomly add a custom data path) would be required to know what the shared data path was already set to.

I would like it better if custom data paths were explicitely created in a specific drectory. However it's an existing issue that your PR does not introduce, so could you just add a comment explaining why you set the custom data path this way and add a TODO to explicitely set custom index paths to be sub directories?

dakrone · 2015-08-10T15:50:18Z

@jpountz pushed new commits for your comments

jpountz · 2015-08-10T15:52:26Z

core/src/main/java/org/elasticsearch/env/Environment.java

+        if (settings.get("path.shared_data") != null) {
+            sharedDataFile = PathUtils.get(cleanPath(settings.get("path.shared_data")));
+        } else {
+            sharedDataFile = homeFile.resolve("data");


Should it resolve to the same as path.data instead?

This is the same as path.data? Or do you mean it should append the cluster name onto it?

I mean if path.data is set eg. in the config/elasticsearch.yml to be another location

Or maybe sharedDataFile should just be null if not set?

Ahh, part of the reason is that path.data can be an array, and so I didn't want to just randomly pick one of the paths

I don't want to set it as null because if we remove custom_paths_enabled we'll now get a NullPointerException if someone tries to use it. I think if we remove custom_paths_enabled we can set this to a better default if you're concerned about it using the data path. Maybe just ${es.path.home}/data/custom or something?

Then maybe it should be null when not set, otherwise we request an unnecessary permission to the security manager, while we should be requesting as few permissions as possible?

I can make it null and add better messaging in the validation I think.

dakrone · 2015-08-10T18:02:21Z

@jpountz pushed a change to make path.shared_data null if unset and add better validation to avoid NPEs

jpountz · 2015-08-11T13:31:18Z

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

@@ -300,7 +300,7 @@ public void run() {
    @Test
    public void testCustomDataPaths() throws Exception {
        String[] dataPaths = tmpPaths();
-        NodeEnvironment env = newNodeEnvironment(dataPaths, Settings.EMPTY);
+        NodeEnvironment env = newNodeEnvironment(dataPaths, "/tmp", Settings.EMPTY);


just checking that this test does not create files in this dir, otherwise we should rather use the java.io.tmpdir?

This doesn't create any files here, it just checks and resolves paths (unit test only)

jpountz · 2015-08-11T13:32:48Z

This looks good to me, but I'd like that someone else gives it a look too. @rjernst maybe you?

rjernst · 2015-08-12T16:45:55Z

LGTM too. Do you think #12776 is quick enough to get in today, so we can remove the old setting altogether for 2.x (since users will be forced to add path.shared_data anyways?)

dakrone · 2015-08-12T16:47:05Z

Do you think #12776 is quick enough to get in today, so we can remove the old setting altogether for 2.x (since users will be forced to add path.shared_data anyways?)

Yeah, this should be a really quick change.

This allows `path.shared_data` to be added to the security manager while still allowing a custom `data_path` for indices using shadow replicas. For example, configuring `path.shared_data: /tmp/foo`, then created an index with: ``` POST /myindex { "index": { "number_of_shards": 1, "number_of_replicas": 1, "data_path": "/tmp/foo/bar/baz", "shadow_replicas": true } } ``` The index will then reside in `/tmp/foo/bar/baz`. `path.shared_data` defaults to `null` if not specified. Resolves elastic#12714 Relates to elastic#11065

dakrone added >enhancement v2.0.0-beta1 review labels Aug 7, 2015

jpountz reviewed Aug 7, 2015
View reviewed changes

jpountz reviewed Aug 10, 2015
View reviewed changes

dakrone mentioned this pull request Aug 10, 2015

Remove node.enable_custom_paths after path.shared_data is added #12776

Closed

dakrone mentioned this pull request Aug 10, 2015

Shadow replica documentation #11497

Closed

jpountz reviewed Aug 11, 2015
View reviewed changes

dakrone force-pushed the shared-data-path branch 2 times, most recently from 0b9ac50 to 6a9c86f Compare August 12, 2015 16:49

dakrone force-pushed the shared-data-path branch from 6a9c86f to ff5ad39 Compare August 12, 2015 16:51

dakrone merged commit ff5ad39 into elastic:master Aug 12, 2015

dakrone added the v2.0.0 label Aug 12, 2015

clintongormley added the :Core/Infra/Settings Settings infrastructure and APIs label Aug 13, 2015

colings86 removed the v2.0.0 label Aug 21, 2015

dakrone deleted the shared-data-path branch March 3, 2016 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `path.shared_data` #12729

Add `path.shared_data` #12729

dakrone commented Aug 7, 2015

jpountz Aug 7, 2015

dakrone Aug 7, 2015

rjernst Aug 7, 2015

dakrone Aug 7, 2015

jpountz Aug 10, 2015

dakrone Aug 10, 2015

dakrone commented Aug 10, 2015

jpountz Aug 10, 2015

dakrone Aug 10, 2015

jpountz Aug 10, 2015

jpountz Aug 10, 2015

dakrone Aug 10, 2015

dakrone Aug 10, 2015

jpountz Aug 10, 2015

dakrone Aug 10, 2015

dakrone commented Aug 10, 2015

jpountz Aug 11, 2015

dakrone Aug 11, 2015

jpountz Aug 11, 2015

jpountz commented Aug 11, 2015

rjernst commented Aug 12, 2015

dakrone commented Aug 12, 2015

Add path.shared_data #12729

Add path.shared_data #12729

Conversation

dakrone commented Aug 7, 2015

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

dakrone commented Aug 10, 2015

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

dakrone commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Aug 11, 2015

rjernst commented Aug 12, 2015

dakrone commented Aug 12, 2015

Add `path.shared_data` #12729

Add `path.shared_data` #12729