From 80f5ccbc2fb9b17cf24b05d508ae7e2ebbefebfc Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Tue, 8 May 2018 17:42:50 +0200 Subject: [PATCH 1/4] [CCR] Added validation checks that were left out of #30120 --- .../ccr/action/FollowExistingIndexAction.java | 122 +++++++++--------- .../xpack/ccr/ShardChangesIT.java | 4 +- .../FollowExistingIndexActionTests.java | 66 ++++++++++ 3 files changed, 133 insertions(+), 59 deletions(-) create mode 100644 x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexAction.java index 2954ffbd440a6..507ed7be98f63 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexAction.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.persistent.PersistentTasksCustomMetaData; import org.elasticsearch.persistent.PersistentTasksService; @@ -223,73 +224,78 @@ protected void doExecute(Request request, ActionListener listener) { */ void start(Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ActionListener handler) { - if (leaderIndexMetadata == null) { - handler.onFailure(new IllegalArgumentException("leader index [" + request.leaderIndex + "] does not exist")); - return; - } - - if (followIndexMetadata == null) { - handler.onFailure(new IllegalArgumentException("follow index [" + request.followIndex + "] does not exist")); - return; - } - - if (leaderIndexMetadata.getNumberOfShards() != followIndexMetadata.getNumberOfShards()) { - handler.onFailure(new IllegalArgumentException("leader index primary shards [" + - leaderIndexMetadata.getNumberOfShards() + "] does not match with the number of " + - "shards of the follow index [" + followIndexMetadata.getNumberOfShards() + "]")); - // TODO: other validation checks - } else { - final int numShards = followIndexMetadata.getNumberOfShards(); - final AtomicInteger counter = new AtomicInteger(numShards); - final AtomicReferenceArray responses = new AtomicReferenceArray<>(followIndexMetadata.getNumberOfShards()); - for (int i = 0; i < numShards; i++) { - final int shardId = i; - String taskId = followIndexMetadata.getIndexUUID() + "-" + shardId; - ShardFollowTask shardFollowTask = new ShardFollowTask(clusterNameAlias, - new ShardId(followIndexMetadata.getIndex(), shardId), - new ShardId(leaderIndexMetadata.getIndex(), shardId), - request.batchSize, request.concurrentProcessors, request.processorMaxTranslogBytes); - persistentTasksService.startPersistentTask(taskId, ShardFollowTask.NAME, shardFollowTask, - new ActionListener>() { - @Override - public void onResponse(PersistentTasksCustomMetaData.PersistentTask task) { - responses.set(shardId, task); - finalizeResponse(); - } + validate(leaderIndexMetadata, followIndexMetadata, request); + final int numShards = followIndexMetadata.getNumberOfShards(); + final AtomicInteger counter = new AtomicInteger(numShards); + final AtomicReferenceArray responses = new AtomicReferenceArray<>(followIndexMetadata.getNumberOfShards()); + for (int i = 0; i < numShards; i++) { + final int shardId = i; + String taskId = followIndexMetadata.getIndexUUID() + "-" + shardId; + ShardFollowTask shardFollowTask = new ShardFollowTask(clusterNameAlias, + new ShardId(followIndexMetadata.getIndex(), shardId), + new ShardId(leaderIndexMetadata.getIndex(), shardId), + request.batchSize, request.concurrentProcessors, request.processorMaxTranslogBytes); + persistentTasksService.startPersistentTask(taskId, ShardFollowTask.NAME, shardFollowTask, + new ActionListener>() { + @Override + public void onResponse(PersistentTasksCustomMetaData.PersistentTask task) { + responses.set(shardId, task); + finalizeResponse(); + } - @Override - public void onFailure(Exception e) { - responses.set(shardId, e); - finalizeResponse(); - } + @Override + public void onFailure(Exception e) { + responses.set(shardId, e); + finalizeResponse(); + } - void finalizeResponse() { - Exception error = null; - if (counter.decrementAndGet() == 0) { - for (int j = 0; j < responses.length(); j++) { - Object response = responses.get(j); - if (response instanceof Exception) { - if (error == null) { - error = (Exception) response; - } else { - error.addSuppressed((Throwable) response); - } + void finalizeResponse() { + Exception error = null; + if (counter.decrementAndGet() == 0) { + for (int j = 0; j < responses.length(); j++) { + Object response = responses.get(j); + if (response instanceof Exception) { + if (error == null) { + error = (Exception) response; + } else { + error.addSuppressed((Throwable) response); } } + } - if (error == null) { - // include task ids? - handler.onResponse(new Response(true)); - } else { - // TODO: cancel all started tasks - handler.onFailure(error); - } + if (error == null) { + // include task ids? + handler.onResponse(new Response(true)); + } else { + // TODO: cancel all started tasks + handler.onFailure(error); } } } - ); - } + } + ); } } } + + + static void validate(IndexMetaData leaderIndex, IndexMetaData followIndex, Request request) { + if (leaderIndex == null) { + throw new IllegalArgumentException("leader index [" + request.leaderIndex + "] does not exist"); + } + + if (followIndex == null) { + throw new IllegalArgumentException("follow index [" + request.followIndex + "] does not exist"); + } + if (leaderIndex.getSettings().getAsBoolean(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), false) == false) { + throw new IllegalArgumentException("leader index [" + request.leaderIndex + "] does not have soft deletes enabled"); + } + + if (leaderIndex.getNumberOfShards() != followIndex.getNumberOfShards()) { + throw new IllegalArgumentException("leader index primary shards [" + leaderIndex.getNumberOfShards() + + "] does not match with the number of shards of the follow index [" + followIndex.getNumberOfShards() + "]"); + } + // TODO: other validation checks + } + } diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java index 02ef001817eb5..34617b236e5d3 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java @@ -19,6 +19,7 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.index.translog.Translog; import org.elasticsearch.persistent.PersistentTasksCustomMetaData; @@ -143,7 +144,8 @@ public void testGetOperationsBasedOnGlobalSequenceId() throws Exception { public void testFollowIndex() throws Exception { final int numberOfPrimaryShards = randomIntBetween(1, 3); - final String leaderIndexSettings = getIndexSettings(numberOfPrimaryShards, Collections.emptyMap()); + final String leaderIndexSettings = getIndexSettings(numberOfPrimaryShards, + Collections.singletonMap(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); assertAcked(client().admin().indices().prepareCreate("index1").setSource(leaderIndexSettings, XContentType.JSON)); final String followerIndexSettings = diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java new file mode 100644 index 0000000000000..1f95128e8cdac --- /dev/null +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java @@ -0,0 +1,66 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.ccr.action; + +import org.elasticsearch.Version; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.equalTo; + +public class FollowExistingIndexActionTests extends ESTestCase { + + public void testValidation() { + FollowExistingIndexAction.Request request = new FollowExistingIndexAction.Request(); + request.setLeaderIndex("index1"); + request.setFollowIndex("index2"); + + { + Exception e = expectThrows(IllegalArgumentException.class, () -> FollowExistingIndexAction.validate(null, null, request)); + assertThat(e.getMessage(), equalTo("leader index [index1] does not exist")); + } + { + IndexMetaData leaderIMD = createIMD("index1", 5); + Exception e = expectThrows(IllegalArgumentException.class, () -> FollowExistingIndexAction.validate(leaderIMD, null, request)); + assertThat(e.getMessage(), equalTo("follow index [index2] does not exist")); + } + { + IndexMetaData leaderIMD = createIMD("index1", 5); + IndexMetaData followIMD = createIMD("index2", 5); + Exception e = expectThrows(IllegalArgumentException.class, + () -> FollowExistingIndexAction.validate(leaderIMD, followIMD, request)); + assertThat(e.getMessage(), equalTo("leader index [index1] does not have soft deletes enabled")); + } + { + IndexMetaData leaderIMD = createIMD("index1", 5, IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"); + IndexMetaData followIMD = createIMD("index2", 4); + Exception e = expectThrows(IllegalArgumentException.class, + () -> FollowExistingIndexAction.validate(leaderIMD, followIMD, request)); + assertThat(e.getMessage(), + equalTo("leader index primary shards [5] does not match with the number of shards of the follow index [4]")); + } + { + IndexMetaData leaderIMD = createIMD("index1", 5, IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"); + IndexMetaData followIMD = createIMD("index2", 5); + FollowExistingIndexAction.validate(leaderIMD, followIMD, request); + } + } + + private static IndexMetaData createIMD(String index, int numShards, String... settings) { + assert settings.length % 2 == 0; + Settings.Builder settingsBuilder = settings(Version.CURRENT); + for (int i = 0; i < settings.length; i += 2) { + settingsBuilder.put(settings[i], settings[i + 1]); + } + return IndexMetaData.builder(index).settings(settingsBuilder) + .numberOfShards(numShards) + .numberOfReplicas(0) + .setRoutingNumShards(numShards).build(); + } + +} From 31908c016535384894c6562a845c521ef4f19a17 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Tue, 15 May 2018 16:51:08 +0200 Subject: [PATCH 2/4] fixed test --- .../resources/rest-api-spec/test/ccr/follow_and_unfollow.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ccr/follow_and_unfollow.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ccr/follow_and_unfollow.yml index 6f17ed4b6eec7..9d8a46ce06532 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ccr/follow_and_unfollow.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ccr/follow_and_unfollow.yml @@ -4,6 +4,10 @@ indices.create: index: foo body: + settings: + index: + soft_deletes: + enabled: true mappings: doc: properties: From cad9e07966fa11e40e5246991a49b1d230aea900 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 16 May 2018 09:19:38 +0200 Subject: [PATCH 3/4] iter --- .../ccr/action/FollowExistingIndexActionTests.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java index 1f95128e8cdac..d876cf2c4d810 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java @@ -7,6 +7,7 @@ import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.test.ESTestCase; @@ -37,7 +38,7 @@ public void testValidation() { assertThat(e.getMessage(), equalTo("leader index [index1] does not have soft deletes enabled")); } { - IndexMetaData leaderIMD = createIMD("index1", 5, IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"); + IndexMetaData leaderIMD = createIMD("index1", 5, new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); IndexMetaData followIMD = createIMD("index2", 4); Exception e = expectThrows(IllegalArgumentException.class, () -> FollowExistingIndexAction.validate(leaderIMD, followIMD, request)); @@ -45,17 +46,16 @@ public void testValidation() { equalTo("leader index primary shards [5] does not match with the number of shards of the follow index [4]")); } { - IndexMetaData leaderIMD = createIMD("index1", 5, IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true"); + IndexMetaData leaderIMD = createIMD("index1", 5, new Tuple<>(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), "true")); IndexMetaData followIMD = createIMD("index2", 5); FollowExistingIndexAction.validate(leaderIMD, followIMD, request); } } - private static IndexMetaData createIMD(String index, int numShards, String... settings) { - assert settings.length % 2 == 0; + private static IndexMetaData createIMD(String index, int numShards, Tuple... settings) { Settings.Builder settingsBuilder = settings(Version.CURRENT); - for (int i = 0; i < settings.length; i += 2) { - settingsBuilder.put(settings[i], settings[i + 1]); + for (Tuple setting : settings) { + settingsBuilder.put(setting.v1(), setting.v2()); } return IndexMetaData.builder(index).settings(settingsBuilder) .numberOfShards(numShards) From 9dbef7793820c9f12c06995b04be0f890248ba31 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 16 May 2018 09:43:00 +0200 Subject: [PATCH 4/4] iter --- .../elasticsearch/xpack/ccr/FollowIndexSecurityIT.java | 9 +++++++++ .../xpack/ccr/action/FollowExistingIndexActionTests.java | 6 +++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/ccr/qa/multi-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java b/x-pack/plugin/ccr/qa/multi-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java index 85648a039c2ab..2fcc0890f2bc0 100644 --- a/x-pack/plugin/ccr/qa/multi-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java +++ b/x-pack/plugin/ccr/qa/multi-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java @@ -64,6 +64,11 @@ public void testFollowIndex() throws Exception { final String indexName2 = "index2"; if (runningAgainstLeaderCluster) { logger.info("Running against leader cluster"); + Settings indexSettings = Settings.builder() + .put("index.soft_deletes.enabled", true) + .build(); + createIndex(indexName1, indexSettings); + createIndex(indexName2, indexSettings); for (int i = 0; i < numDocs; i++) { logger.info("Indexing doc [{}]", i); index(indexName1, Integer.toString(i), "field", i); @@ -169,6 +174,10 @@ private static Map toMap(String response) { return XContentHelper.convertToMap(JsonXContent.jsonXContent, response, false); } + protected static void createIndex(String name, Settings settings) throws IOException { + createIndex(name, settings, ""); + } + protected static void createIndex(String name, Settings settings, String mapping) throws IOException { assertOK(adminClient().performRequest(HttpPut.METHOD_NAME, name, Collections.emptyMap(), new StringEntity("{ \"settings\": " + Strings.toString(settings) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java index d876cf2c4d810..5c6e3e3f9543d 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowExistingIndexActionTests.java @@ -52,10 +52,10 @@ public void testValidation() { } } - private static IndexMetaData createIMD(String index, int numShards, Tuple... settings) { + private static IndexMetaData createIMD(String index, int numShards, Tuple... settings) { Settings.Builder settingsBuilder = settings(Version.CURRENT); - for (Tuple setting : settings) { - settingsBuilder.put(setting.v1(), setting.v2()); + for (Tuple setting : settings) { + settingsBuilder.put((String) setting.v1(), (String) setting.v2()); } return IndexMetaData.builder(index).settings(settingsBuilder) .numberOfShards(numShards)