add is-write-index flag to aliases #30942

talevy · 2018-05-29T21:56:18Z

This commit adds the is-write-index flag for aliases.
It allows requests to set the flag, and responses to display the flag.
It does not validate and/or affect any indexing/getting/updating behavior
of Elasticsearch -- this will be done in a follow-up PR.

This is a follow-up to: #30703
It made more sense to split the above PR into two PRs. One for introducing the is_write_index
flag, and another to use it for validation and index resolution.

elasticmachine · 2018-05-29T21:56:20Z

Pinging @elastic/es-core-infra

bleskes

Thanks @talevy , much smaller :) left some comments.

bleskes · 2018-05-30T15:27:37Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_alias/all_path_options.yml

@@ -27,9 +27,9 @@ setup:
      indices.get_alias:
        name: alias

-  - match: {test_index1.aliases.alias: {}}
+  - match: {test_index1.aliases.alias: { is_write_index: false }}


why are these false? this is the only index in this alias so it should be true?

bleskes · 2018-05-30T15:30:22Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/Alias.java

@@ -60,6 +62,8 @@
    @Nullable
    private String searchRouting;

+    private boolean writeIndex;


How do we specify "default"? don't we want a Boolean here?

bleskes · 2018-05-30T15:49:09Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

@@ -516,7 +516,7 @@ public ClusterState execute(ClusterState currentState) throws Exception {
                }
                for (Alias alias : request.aliases()) {
                    AliasMetaData aliasMetaData = AliasMetaData.builder(alias.name()).filter(alias.filter())
-                        .indexRouting(alias.indexRouting()).searchRouting(alias.searchRouting()).build();
+                        .indexRouting(alias.indexRouting()).searchRouting(alias.searchRouting()).writeIndex(alias.isWriteIndex()).build();


we need to resolve the is write index here (for when it's not specified).

bleskes · 2018-05-30T15:53:00Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasAction.java

@@ -112,7 +121,8 @@ boolean removeIndex() {
        boolean apply(NewAliasValidator aliasValidator, MetaData.Builder metadata, IndexMetaData index) {
            aliasValidator.validate(alias, indexRouting, filter);
            AliasMetaData newAliasMd = AliasMetaData.newAliasMetaDataBuilder(alias).filter(filter).indexRouting(indexRouting)
-                    .searchRouting(searchRouting).build();
+                    .searchRouting(searchRouting).writeIndex(writeIndex).build();


what happens if writeIndex is null?

docs/reference/indices/aliases.asciidoc

+==== Write Index
+
+It is possible to associate the index pointed to by an alias as the write index.
+When specified, all index and update requests against an alias that point to multiple


This commit adds the is-write-index flag for aliases. It allows requests to set the flag, and responses to display the flag. It does not validate and/or affect any indexing/getting/updating behavior of Elasticsearch -- this will be done in a follow-up PR.

talevy · 2018-06-01T02:58:58Z

@bleskes thanks for the initial review. As I mentioned outside of Github, I believe we were misaligned in terms of how to break apart the original PR that both added the flag, and leveraged it to resolve index name expressions.

I've updated this PR to re-add validation and auto-upgrading of the is_write_index. This means that alias-index configurations with is_write_index=null in both alias-add-actions and index-creations will be set to true when there is no existing write index for that alias. In both cases that a promotion to true is not possible, false will be set.

Although AliasMetaData stores this value as a Boolean, any cluster-state changes that occur via index-creation or alias-updates will resolve this value to either true or false. AliasMetaData#writeIndex must stay a Boolean because of its use in templates. Templates need not define this field explicitly, and no smart unboxing should occur.

I've also added some unit tests that should account for these scenarios. (still iterating to catch all the CI tests that need updating).

rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_alias/all_path_options.yml

-  - match: {foo.aliases.alias: {}}
+  - match: {test_index1.aliases.alias: { is_write_index: true }}
+  - match: {test_index2.aliases.alias: { is_write_index: false }}
+  - match: {foo.aliases.alias: { is_write_index: false }}

 ---
 "put alias in * index":


talevy · 2018-06-05T03:01:48Z

@bleskes I've updated to reflect the changes we mentioned offline

move all validation logic to run at the time that MetaData is built
default is_write_index (null) to true if-and-only-if there are no other aliases, false otherwise

I believe I am still missing some test coverage, but I think it should be good enough to review.
I do not feel comfortable with how complex MetaData#build is, but I decided to push back on opening at can of worms for this PR.

bleskes

Thx @talevy . I left some suggestions.

bleskes · 2018-06-08T14:34:21Z

docs/reference/indices/aliases.asciidoc

+// CONSOLE
+// TEST[s/^/PUT test\nPUT test2\n/]
+
+It is important to note that the order of these actions is important. The original write index


This is tricky - with our new model the order is not important but it is that you remove an explicit flag. I wonder if we should add a section over the "default" behavior and illustrate some examples.

oh, woops. I thought I went through this. will update since it is left-over and currently wrong

bleskes · 2018-06-08T14:41:37Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasMetaData.java

@@ -308,6 +337,8 @@ public static void toXContent(AliasMetaData aliasMetaData, XContentBuilder build
                builder.field("search_routing", aliasMetaData.searchRouting());
            }

+            builder.field("is_write_index", Boolean.TRUE.equals(aliasMetaData.writeIndex()));


I think we want just aliasMetaData.writeIndex() here?

The aspect of Clint's original proposal that I liked was that we would always show this field and make it clear to the user which value we defaulted to. Having it be null feels confusing.

make it clear to the user which value we defaulted to

That's my point - we don't know in this point. I agree that outputting null will be confusing hence my suggestion which will completely omit the field if not set.

bleskes · 2018-06-08T15:21:11Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasOrIndex.java

@@ -134,8 +149,11 @@ public AliasMetaData getFirstAliasMetaData() {
            return referenceIndexMetaDatas.get(0).getAliases().get(aliasName);
        }

-        void addIndex(IndexMetaData indexMetaData) {
+        void addIndex(IndexMetaData indexMetaData, @Nullable Boolean isWriteIndex) {


Instead of doing this like this maybe only do it at the end? add a method called "computeAndValidateWriteIndex" and let that one do two things:

scan the all index metadata and extracts the writeIndex, if any

validate we have no two indices which are marked with the index_write flags.

you can use org.apache.lucene.util.SetOnce to store the writeIndex reference. Then you can assert that once computeAndValidateWriteIndex is called you can't call addIndex anymore.

bleskes · 2018-06-08T15:21:23Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java

+     * @param aliasAndIndexLookup The current state of aliases and indices in the cluster state
+     * @throws IllegalStateException if any alias has more than one write index
+     */
+    public static void validateAliasWriteOnly(Map<String, AliasOrIndex> aliasAndIndexLookup) {


see my other comment.

bleskes · 2018-06-08T15:23:51Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

@@ -701,7 +702,8 @@ private static IndexNotFoundException indexNotFoundException(String expression)
                if (context.getOptions().ignoreAliases()) {
                    return metaData.getAliasAndIndexLookup().entrySet().stream()
                            .filter(e -> e.getValue().isAlias() == false)
-                            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+                            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue,
+                                (v1, v2) -> {throw new IllegalStateException("no duplicate indices should exist");}, TreeMap::new));


out of curiosity - why did you add this?

oh woops. this was not meant to stay. I added this to debug where the ordering of indices was being lost. will revert

bleskes · 2018-06-08T15:24:28Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

+     * @param alias the alias to search for a write-index with
+     * @return the write-index {@link Index} for <code>alias</code>, null if no write-index is configured
+     */
+    public Index getWriteIndex(String alias) {


this is usage? I think we should remove it and use it in the follow up PRs we're planning?

I will double check, but I believe it will be used by the index-name resolver when used for indexing. It is not used yet since we haven't implemented that yet

bleskes · 2018-06-08T15:25:16Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

+            AliasValidator.validateAliasWriteOnly(aliasAndIndexLookup);
+
+            // set any `is_write_index == null` to their appropriate boolean values
+            for (AliasOrIndex aliasOrIndex : aliasAndIndexLookup.values()) {


see other comment - this should all be done in buildAliasAndIndexLookup

bleskes · 2018-06-08T15:26:06Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

+
+
+            // build all concrete indices arrays:
+            // TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices.


I believe this is about memory allocation - I think we can discuss this but in a different issue.

I didn't write this

talevy · 2018-06-12T04:52:21Z

hey @bleskes, I've updated the build logic to behave more like a one-pass build of the aliasAndIndexLookup + manipulating the IndexMetaData of the cluster, itself.

one thing I am debugging is how this behavior is affecting snapshot restoration. Since one can restore and rename index names, the aliasmetadata now has multiple copies of is_write_index: true. So restoration is failing

server/src/main/java/org/elasticsearch/cluster/metadata/AliasMetaData.java

+            writeIndex = in.readOptionalBoolean();
+        } else {
+            writeIndex = null;
+        }
    }

    public static Diff<AliasMetaData> readDiffFrom(StreamInput in) throws IOException {


talevy · 2018-06-14T15:16:42Z

I've updated to add more tests and removed unused imports, ready for another review @bleskes. thanks!

bleskes

LGTM. looks great @talevy

talevy · 2018-06-15T15:45:06Z

thanks! I will probably finalize the back-porting dance early next week in the hopes of not breaking a CI run on a Friday

albertzaharovits · 2018-06-17T09:24:22Z

@talevy I have pushed 5b94afd
It fixes a minor test fault that manifests reproducible with:

 ./gradlew :server:test -Dtests.seed=22BB040A257F9A41 -Dtests.class=org.elasticsearch.cluster.metadata.ToAndFromJsonMetaDataTests -Dtests.method="testSimpleJsonFromAndTo" -Dtests.security.manager=true -Dtests.locale=pl-PL -Dtests.timezone=PRT
ERROR   0.33s | ToAndFromJsonMetaDataTests.testSimpleJsonFromAndTo <<< FAILURES!
   > Throwable #1: java.lang.IllegalStateException: alias [alias2] has more than one write index [test11,test12]
   >    at __randomizedtesting.SeedInfo.seed([22BB040A257F9A41:FE049499C585A170]:0)
   >    at org.elasticsearch.cluster.metadata.AliasOrIndex$Alias.computeAndValidateWriteIndex(AliasOrIndex.java:168)
   >    at org.elasticsearch.cluster.metadata.MetaData$Builder.lambda$buildAliasAndIndexLookup$1(MetaData.java:1078)
   >    at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)

which I have stumbled upon on my PR CI https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/11893/console .

talevy · 2018-06-17T13:04:16Z

thanks @albertzaharovits! sorry for the noise

* master: Add get stored script and delete stored script to high level REST API - post backport fix Add get stored script and delete stored script to high level REST API (#31355) Core: Combine Action and GenericAction (#31405) Fix reference to XContentBuilder.string() (#31337) Avoid sending duplicate remote failed shard requests (#31313) Fix defaults in GeoShapeFieldMapper output (#31302) RestAPI: Reject forcemerge requests with a body (#30792) Packaging: Remove windows bin files from the tar distribution (#30596) Docs: Use the default distribution to test docs (#31251) [DOCS] Adds testing for security APIs (#31345) Clarify that IP range data can be specified in CIDR notation. (#31374) Use system context for cluster state update tasks (#31241) Percentile/Ranks should return null instead of NaN when empty (#30460) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) [Test] Fix :example-plugins:rest-handler on Windows Expose lucene's RemoveDuplicatesTokenFilter (#31275) Reload secure settings for plugins (#31383) Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381) Ensure we don't use a remote profile if cluster name matches (#31331) [TEST] Double write alias fault (#30942) [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC SQL: Fix rest endpoint names in node stats (#31371) Support for remote path in reindex api - post backport fix Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Support for remote path in reindex api (#31290) Add byte array pooling to nio http transport (#31349) Remove trial status info from start trial doc (#31365) [DOCS] Adds links to release notes and highlights add is-write-index flag to aliases (#30942) Add rollover-creation-date setting to rolled over index (#31144) [ML] Hold ML filter items in sorted set (#31338) [Tests] Fix edge case in ScriptedMetricAggregatorTests (#31357)

This commit adds the is-write-index flag for aliases. It allows requests to set the flag, and responses to display the flag. It does not validate and/or affect any indexing/getting/updating behavior of Elasticsearch -- this will be done in a follow-up PR.

* add is-write-index flag to aliases (#30942) This commit adds the is-write-index flag for aliases. It allows requests to set the flag, and responses to display the flag. It does not validate and/or affect any indexing/getting/updating behavior of Elasticsearch -- this will be done in a follow-up PR. * [TEST] Double write alias fault (#30942)

* 6.x: [DOCS] Omit shard failures assertion for incompatible responses (#31430) [DOCS] Move licensing APIs to docs (#31445) backport of: add is-write-index flag to aliases (#30942) (#31412) backport of: Add rollover-creation-date setting to rolled over index (#31144) (#31413) [Docs] Extend Homebrew installation instructions (#28902) [Docs] Mention ip_range datatypes on ip type page (#31416) Multiplexing token filter (#31208) Fix use of time zone in date_histogram rewrite (#31407) Revert "Mute DefaultShardsIT#testDefaultShards test" [DOCS] Fixes code snippet testing for machine learning (#31189) Security: fix joining cluster with production license (#31341) [DOCS] Updated version in Info API example [DOCS] Moves the info API to docs (#31121) Revert "Increasing skip version for failing test on 6.x" Preserve response headers on cluster update task (#31421) [DOCS] Add code snippet testing for more ML APIs (#31404) Docs: Advice for reindexing many indices (#31279)

talevy added >enhancement review :Data Management/Indices APIs APIs to create and manage indices and templates labels May 29, 2018

talevy requested a review from bleskes May 29, 2018 21:56

talevy force-pushed the is-write-index-setting branch 3 times, most recently from a47b7d7 to 54d952a Compare May 30, 2018 02:21

bleskes suggested changes May 30, 2018

View reviewed changes

bleskes reviewed May 30, 2018

View reviewed changes

docs/reference/indices/aliases.asciidoc

==== Write Index

It is possible to associate the index pointed to by an alias as the write index.

When specified, all index and update requests against an alias that point to multiple

This comment was marked as resolved.

Sign in to view

talevy added 2 commits May 31, 2018 13:26

cleanup

7baf86b

talevy force-pushed the is-write-index-setting branch from 54d952a to 7baf86b Compare June 1, 2018 01:46

fix unit tests

19315ff

update docs and tests

440fed4

talevy commented Jun 1, 2018

View reviewed changes

talevy added 2 commits June 4, 2018 19:59

change to validate at MetaData.build

135d7c1

Merge branch 'master' into is-write-index-setting

5bb7217

talevy added 2 commits June 5, 2018 08:13

Merge branch 'master' into is-write-index-setting

847bbe6

add writeIndex to ToAndFromJsonMetaDataTests

32ca015

bleskes suggested changes Jun 8, 2018

View reviewed changes

talevy added 4 commits June 11, 2018 17:22

Merge branch 'master' into is-write-index-setting

07188f3

refactor as to not re-build aliasAndIndexLookup

01fab60

migrate validation tests and setonce writeIndex

c7a3a96

Merge branch 'master' into is-write-index-setting

8df9bc8

talevy commented Jun 12, 2018

View reviewed changes

fix SharedClusterSnapshotRestoreIT#testRenameOnRestore

4e898c1

talevy added 5 commits June 13, 2018 12:12

Merge branch 'master' into is-write-index-setting

ba1162c

split out is_write_index check in yaml test and skip if run before 7.0

a1391b2

add test for write index swapping

91deba4

Merge branch 'master' into is-write-index-setting

1629aef

remove unused imports

f8dba0c

bleskes approved these changes Jun 15, 2018

View reviewed changes

talevy merged commit 3b70e94 into elastic:master Jun 15, 2018

talevy deleted the is-write-index-setting branch June 15, 2018 15:45

albertzaharovits added a commit that referenced this pull request Jun 17, 2018

[TEST] Double write alias fault (#30942)

5b94afd

albertzaharovits mentioned this pull request Jun 17, 2018

Reload secure settings for plugins #31383

Merged

talevy mentioned this pull request Jun 18, 2018

backport of: add is-write-index flag to aliases (#30942) #31412

Merged

talevy added v7.0.0 and removed review labels Jun 18, 2018

talevy mentioned this pull request Jun 18, 2018

extend is-write-index serialization support to 6.4 #31415

Merged

talevy pushed a commit to talevy/elasticsearch that referenced this pull request Jun 19, 2018

[TEST] Double write alias fault (elastic#30942)

6429221

This was referenced Jul 10, 2018

Metadata cannot reject aliases pointing to multiple indices when resolving routing #31936

Closed

[meta] Alias is_write_index feature tracking #31959

Closed

jbaiera mentioned this pull request Oct 2, 2018

Connector should validate 'is_write_index' flag on aliases during write operations elastic/elasticsearch-hadoop#1197

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add is-write-index flag to aliases #30942

add is-write-index flag to aliases #30942

talevy commented May 29, 2018

elasticmachine commented May 29, 2018

bleskes left a comment

bleskes May 30, 2018

bleskes May 30, 2018

bleskes May 30, 2018

bleskes May 30, 2018

This comment was marked as resolved.

talevy commented Jun 1, 2018

This comment was marked as resolved.

talevy commented Jun 5, 2018 •

edited

bleskes left a comment

bleskes Jun 8, 2018

talevy Jun 8, 2018

bleskes Jun 8, 2018 •

edited

talevy Jun 8, 2018

bleskes Jun 8, 2018

bleskes Jun 8, 2018

bleskes Jun 8, 2018

bleskes Jun 8, 2018

talevy Jun 8, 2018

bleskes Jun 8, 2018

talevy Jun 8, 2018

bleskes Jun 8, 2018

bleskes Jun 8, 2018

talevy Jun 8, 2018

talevy commented Jun 12, 2018

This comment was marked as resolved.

This comment was marked as resolved.

talevy commented Jun 14, 2018

bleskes left a comment

talevy commented Jun 15, 2018

albertzaharovits commented Jun 17, 2018 •

edited

talevy commented Jun 17, 2018



		// build all concrete indices arrays:
		// TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices.

add is-write-index flag to aliases #30942

add is-write-index flag to aliases #30942

Conversation

talevy commented May 29, 2018

elasticmachine commented May 29, 2018

bleskes 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

This comment was marked as resolved.

talevy commented Jun 1, 2018

This comment was marked as resolved.

talevy commented Jun 5, 2018 • edited

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes Jun 8, 2018 • edited

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

talevy commented Jun 12, 2018

This comment was marked as resolved.

This comment was marked as resolved.

talevy commented Jun 14, 2018

bleskes left a comment

Choose a reason for hiding this comment

talevy commented Jun 15, 2018

albertzaharovits commented Jun 17, 2018 • edited

talevy commented Jun 17, 2018

talevy commented Jun 5, 2018 •

edited

bleskes Jun 8, 2018 •

edited

albertzaharovits commented Jun 17, 2018 •

edited