[CCR] Add create and follow api #30602

martijnvg · 2018-05-15T06:48:37Z

This PR adds a new api that creates the follow index and starts the follow changes from leader index into the newly created follow index. The api looks the same as the existing follow index api, with the big difference that it creates the follow index.

The create and follow api, creates a follow index based on the IndexMetaData of the leader index, then waits for the shards to become available and the delegate to the existing follow index api.

Relates to #30102

and fixed tests

elasticmachine · 2018-05-15T06:48:38Z

Pinging @elastic/es-distributed

* es/ccr: (37 commits) Default to one shard (elastic#30539) Unmute IndexUpgradeIT tests Forbid expensive query parts in ranking evaluation (elastic#30151) Docs: Update HighLevelRestClient migration docs (elastic#30544) Clients: Switch to new performRequest (elastic#30543) [TEST] Fix typo in MovAvgIT test Add missing dependencies on testClasses (elastic#30527) [TEST] Mute ML test that needs updating to following ml-cpp changes Document woes between auto-expand-replicas and allocation filtering (elastic#30531) Moved tokenizers to analysis common module (elastic#30538) Adjust copy settings versions Mute ShrinkIndexIT suite SQL: SYS TABLES ordered according to *DBC specs (elastic#30530) Deprecate not copy settings and explicitly disallow (elastic#30404) [ML] Improve state persistence log message Build: Add mavenPlugin cluster configuration method (elastic#30541) Re-enable FlushIT tests Bump Gradle heap to 2 GB (elastic#30535) SQL: Use request flavored methods in tests (elastic#30345) Suppress hdfsFixture if there are spaces in the path (elastic#30302) ...

* es/ccr: (75 commits) Preserve REST client auth despite 401 response (elastic#30558) [test] packaging: add windows boxes (elastic#30402) Make xpack modules instead of a meta plugin (elastic#30589) Mute ShrinkIndexIT [ML] DeleteExpiredDataAction should use client with origin (elastic#30646) Reindex: Fixed typo in assertion failure message (elastic#30619) [DOCS] Fixes list of unconverted snippets in build.gradle [DOCS] Reorganizes RBAC documentation SQL: Remove dependency for server's version from JDBC driver (elastic#30631) Test: increase search logging for LicensingTests Adjust serialization version in IndicesOptions [TEST] Fix compilation Remove version argument in RangeFieldType (elastic#30411) Remove unused DirectoryUtils class. (elastic#30582) Mitigate date histogram slowdowns with non-fixed timezones. (elastic#30534) Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (elastic#29594) Removes AwaitsFix on IndicesOptionsTests Template upgrades should happen in a system context (elastic#30621) Fix bug in BucketMetrics path traversal (elastic#30632) Fixes IndiceOptionsTests to serialise correctly (elastic#30644) ...

jasontedor

Looks great; I left a few comments. I will hold off approving as I have a couple of questions.

jasontedor · 2018-05-20T17:37:04Z

x-pack/plugin/ccr/qa/multi-cluster-with-security/roles.yml

@@ -2,7 +2,7 @@ ccruser:
  cluster:
    - manage_ccr
  indices:
-    - names: [ 'index1' ]
+    - names: [ 'index1', 'index3' ]


I think more meaningful names are in order here? When I look at this and look at FollowIndexSecurityIT it takes some mental energy to keep track of what and where index[1234] are.

jasontedor · 2018-05-20T17:37:47Z

...i-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java

+            logger.info("Running against leader cluster");
+            Settings indexSettings = Settings.builder()
+                .put("index.soft_deletes.enabled", true)
+                .build();


I think this will all fit onto one line.

jasontedor · 2018-05-20T17:38:29Z

...i-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java

+    public void testCreateAndFollowIndex() throws Exception {
+        final int numDocs = 16;
+        final String indexName1 = "index3";
+        final String indexName2 = "index4";


See my comment above about the index names; this is especially onerous with the variable names not being lined up with the concrete index names (e.g., indexName1 is lined up with index3).

jasontedor · 2018-05-20T17:42:26Z

...i-cluster-with-security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java

@@ -111,6 +111,53 @@ public void testFollowIndex() throws Exception {
        }
    }

+    public void testCreateAndFollowIndex() throws Exception {


I do think that two separate tests would be better. One that tests against an index with appropriate permissions, and one that tests against an index without appropriate permissions. Most of the code can be shared between the two methods, it is only about what is asserted at the end that needs to change.

Agreed, I'm going to change this.

jasontedor · 2018-05-20T17:51:14Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CreateAndFollowIndexAction.java

+
+                        Settings.Builder settingsBuilder = Settings.builder();
+                        settingsBuilder.put(leaderIndexMetaData.getSettings());
+                        // TODO: do we want leader and follow index to have the same UUID?


As you note one line below, we can not do this if the indices are intra-cluster yet I am not sure if I see any advantage to doing this if the indices are inter-cluster.

Thanks for checking, I just wanted to point this out during the review. I will remove the todo.

jasontedor · 2018-05-20T17:54:06Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CreateAndFollowIndexAction.java

+                        imdBuilder.settings(settingsBuilder);
+                        imdBuilder.index(request.getFollowRequest().getFollowIndex());
+                        for (int shardId = 0 ; shardId < leaderIndexMetaData.getNumberOfShards(); shardId++) {
+                            imdBuilder.putInSyncAllocationIds(shardId, new HashSet<>());


I don't think this is strictly needed (it should be the default; see IndexMetaData#build). Did you find otherwise?

Yes, I did. Shards of the follow index would never get started (allocation status no_valid_shard_copy).

Okay; this is happening because we copy all of the leader index metadata (which brings along with it the in-sync shard copies on the leader). Then, we have to overwrite with an empty in-sync shard set. I don't think we should do this, I lean towards thinking that we should only copy explicitly what we need from the leader index rather than copying everything and overwriting.

I lean towards thinking that we should only copy explicitly what we need from the leader index rather than copying everything and overwriting.

+1 , at least at the current stage where we focus on data and not cluster level metadata (with the exception of mappings).

jasontedor · 2018-05-20T18:00:12Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CreateAndFollowIndexAction.java

+                        }
+                        mdBuilder.put(imdBuilder.build(), false);
+                    } else {
+                        mdBuilder.put(leaderIndexMetaData, false);


I don't think this case is possible; what am I missing?

True, this is something that I should have removed.

jasontedor · 2018-05-20T18:03:21Z

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java

@@ -338,6 +318,28 @@ public void testFollowNonExistentIndex() throws Exception {
        };
    }

+    private void unFollowIndex(String index) throws Exception {


Nit: unFollow -> unfollow

* es/ccr: (50 commits) Reduce CLI scripts to one-liners (elastic#30759) SQL: Preserve scoring in bool queries (elastic#30730) QA: Switch rolling upgrade to 3 nodes (elastic#30728) [TEST] Enable DEBUG logging on testAutoQueueSizingWithMax [ML] Don't install empty ML metadata on startup (elastic#30751) Add assertion on removing copy_settings (elastic#30748) bump lucene version for 6_3_0 [DOCS] Mark painless execute api as experimental (elastic#30710) disable annotation processor for docs (elastic#30610) Add more script contexts (elastic#30721) Fix default shards count in create index docs (elastic#30747) Mute testCorruptFileThenSnapshotAndRestore Scripting: Remove getDate methods from ScriptDocValues (elastic#30690) Upgrade to Lucene-7.4.0-snapshot-59f2b7aec2 (elastic#30726) [Docs] Fix single page :docs:check invocation (elastic#30725) Docs: Add uptasticsearch to list of clients (elastic#30738) [DOCS] Removes out-dated x-pack/docs/en/index.asciidoc [DOCS] Removes redundant index.asciidoc files (elastic#30707) [TEST] Reduce forecast overflow to disk test memory limit (elastic#30727) Plugins: Remove meta plugins (elastic#30670) ...

martijnvg · 2018-05-22T13:01:46Z

@jasontedor I've updated the PR.

jasontedor · 2018-05-24T11:45:09Z

@martijnvg I left one more comment. The overall change looks good but we should come to a resolution on how we will handle sourcing index metadata from the leader (copy and overwrite some fields versus only copy what is needed).

martijnvg · 2018-05-24T15:30:14Z

@jasontedor I've addressed your comment: c1f8106

jasontedor

Looks good.

* es/ccr: (55 commits) [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (elastic#30820) Painless: Types Section Clean Up (elastic#30283) Add support for indexed shape routing in geo_shape query (elastic#30760) [test] java tests for archive packaging (elastic#30734) Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813) [DOCS] Fix more edit URLs in Stack Overview (elastic#30704) Use correct cluster state version for node fault detection (elastic#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (elastic#30797) [TEST] Don't expect acks when isolating nodes Mute CorruptedFileIT in CCR Add a `format` option to `docvalue_fields`. (elastic#29639) Fixes UpdateSettingsRequestStreamableTests mutate bug Mustes {p0=snapshot.get_repository/10_basic/*} YAML test Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (elastic#30743) ...

martijnvg · 2018-05-25T07:16:05Z

@elasticmachine test this please

martijnvg · 2018-05-25T11:23:59Z

@elasticmachine test this please

martijnvg · 2018-05-25T12:56:15Z

@elasticmachine test this please

martijnvg · 2018-05-25T13:49:19Z

@elasticmachine test this please

martijnvg · 2018-05-26T05:49:30Z

@elasticmachine test this please

Also renamed FollowExisting* internal names to just Follow* and fixed tests

Bug was introduced when create and follow api was added in elastic#30602

Bug was introduced when create and follow api was added in #30602

martijnvg added 2 commits May 15, 2018 08:08

Added create and follow api.

9bb2413

Renamed FollowExisting* internal names to just Follow*

d79989a

and fixed tests

martijnvg added review :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels May 15, 2018

martijnvg requested a review from jasontedor May 15, 2018 06:48

martijnvg added 5 commits May 15, 2018 11:30

Merge remote-tracking branch 'es/ccr' into ccr_create_and_follow

9ca0376

iter after merge

491b1a9

fixed checkstyle violation

531889a

jasontedor reviewed May 20, 2018

View reviewed changes

martijnvg added 2 commits May 22, 2018 11:48

iter

6ef4681

Create a clean IMD instance and just copy over settings and mappings.

c1f8106

jasontedor approved these changes May 25, 2018

View reviewed changes

Merge remote-tracking branch 'es/ccr' into ccr_create_and_follow

27da0e3

Merge remote-tracking branch 'es/ccr' into ccr_create_and_follow

ab53fcf

martijnvg merged commit e477147 into elastic:ccr May 26, 2018

martijnvg added a commit that referenced this pull request May 26, 2018

[CCR] Add create and follow api (#30602)

702ec19

Also renamed FollowExisting* internal names to just Follow* and fixed tests

martijnvg mentioned this pull request May 27, 2018

[CCR] Also copy routing_num_shards from leader to follow index. #30894

Merged

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 28, 2018

[CCR] Also copy routing_num_shards from leader to follow index.

fde649e

Bug was introduced when create and follow api was added in elastic#30602

martijnvg added a commit that referenced this pull request May 31, 2018

[CCR] Also copy routing_num_shards from leader to follow index. (#30894)

a82f2e3

Bug was introduced when create and follow api was added in #30602

martijnvg added a commit that referenced this pull request May 31, 2018

[CCR] Also copy routing_num_shards from leader to follow index. (#30894)

4da2299

Bug was introduced when create and follow api was added in #30602

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Add create and follow api #30602

[CCR] Add create and follow api #30602

martijnvg commented May 15, 2018

elasticmachine commented May 15, 2018

jasontedor left a comment

jasontedor May 20, 2018

jasontedor May 20, 2018

jasontedor May 20, 2018

jasontedor May 20, 2018

martijnvg May 22, 2018

jasontedor May 20, 2018

martijnvg May 22, 2018

jasontedor May 20, 2018

martijnvg May 22, 2018

jasontedor May 24, 2018

bleskes May 24, 2018

jasontedor May 20, 2018

martijnvg May 22, 2018

jasontedor May 20, 2018

martijnvg commented May 22, 2018

jasontedor commented May 24, 2018

martijnvg commented May 24, 2018

jasontedor left a comment

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 26, 2018

[CCR] Add create and follow api #30602

[CCR] Add create and follow api #30602

Conversation

martijnvg commented May 15, 2018

elasticmachine commented May 15, 2018

jasontedor 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

martijnvg commented May 22, 2018

jasontedor commented May 24, 2018

martijnvg commented May 24, 2018

jasontedor left a comment

Choose a reason for hiding this comment

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 25, 2018

martijnvg commented May 26, 2018