Add rollover-creation-date setting to rolled over index by talevy · Pull Request #31144 · elastic/elasticsearch

talevy · 2018-06-06T16:20:06Z

This PR introduces a new section to IndexMetaData for storing information about
rollover actions that took place against a specific index.

Information stored

which alias was used to rollover
when the index was rolled over with that alias
which conditions were met

Closes #30887

This PR introduces a new index setting `index.rollover.creation_date` much like the `index.creation_date`, it captures the approximate time that the index was rolled over to a new one.

elasticmachine · 2018-06-06T16:20:08Z

Pinging @elastic/es-core-infra

talevy · 2018-06-06T16:24:26Z

Notes for the reviewer:

I was looking for a way to make the alias-swap + setting update to be atomic, but could not find a clean way
I was looking to add unit tests, but it seems like the approach taken in Rollover is not very unit-test friendly and has, instead, chosen the RolloverIT route to verify behavior.
It was not clear whether new settings should be added to IndexMetaData, or IndexSettings.
I was interested in providing a supplier for the current time, but I don't see that index.creation_date cares much for that, so I just use System millis hardcoded. Since the exact time is not necessarily important, and a close approximate is good, I think this should be fine for now.

jasontedor

I do not think that we should use a setting for this. I think that we should have index metadata that captures:

the alias that was rolled over
the condition that caused the rollover
when this occurred

@talevy Can you look into the feasibility of this?

talevy · 2018-06-07T16:24:43Z

yes @jasontedor , I see no reason this shouldn't be doable. I guess I was just following in line with the resize-settings strategy.

you mean to add properties to IndexMetaData class itself to hold this data. Makes sense since it is for internal use when IndexMetaData is available.

This reverts commit 7817a9f.

This commit introduces a new property to IndexMetaData called RolloverInfo. This object contains a map containing the aliases that were used to rollover the related index, which conditions were met, and at what time the rollover took place.

talevy · 2018-06-11T23:58:03Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MaxAgeCondition.java

    }
+
+    public static MaxAgeCondition fromXContent(XContentParser parser) throws IOException {
+        parser.nextToken();


what am I doing wrong in parsing that led to me wind up calling this here?

talevy · 2018-06-12T00:00:37Z

heya @jasontedor, I've updated the PR to introduce the object you recommended.

I plan on doing one more iteration on this, but I believe it is ready for initial review. thanks!

EDIT: iterations made

talevy · 2018-06-12T00:01:26Z

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

                });
            inSyncAllocationIds = DiffableUtils.readImmutableOpenIntMapDiff(in, DiffableUtils.getVIntKeySerializer(),
                DiffableUtils.StringSetValueSerializer.getInstance());
+            rolloverInfos = DiffableUtils.readImmutableOpenMapDiff(in, DiffableUtils.getStringKeySerializer(), RolloverInfo::new,


hmm. these diffs should be behind version checks, right?

jasontedor

It looks pretty good; I left a few comments. They are minor.

jasontedor · 2018-06-13T01:42:51Z

...r/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java

+                                            @Override
+                                            public ClusterState execute(ClusterState currentState) {
+                                                RolloverInfo rolloverInfo = new RolloverInfo(rolloverRequest.getAlias(), metConditions,
+                                                    System.currentTimeMillis());


I think that this should be threadPool.absoluteTimeMillis().

jasontedor · 2018-06-13T01:43:34Z

server/src/main/java/org/elasticsearch/client/transport/TransportClient.java

+            IndicesModule indicesModule = new IndicesModule(Collections.emptyList());
            List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
            entries.addAll(NetworkModule.getNamedWriteables());
+            entries.addAll(indicesModule.getNamedWriteables());


A small nit here; we have the ordering searchModule, indicesModule for the local variables; can we do the same on the invocation to getNamedWritables?

jasontedor · 2018-06-13T01:44:30Z

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

        if (!inSyncAllocationIds.equals(that.inSyncAllocationIds)) {
            return false;
        }
+        if (!rolloverInfos.equals(that.rolloverInfos)) {


== false (#30697) (I see there are other places in the source that are not like this but they can stay.)

jasontedor · 2018-06-13T01:47:30Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverIT.java

        assertTrue(newIndex.getAliases().containsKey("test_alias"));
+        assertThat(oldIndex.getRolloverInfos().size(), equalTo(1));
+        assertThat(oldIndex.getRolloverInfos().get("test_alias").getAlias(), equalTo("test_alias"));
+        assertThat(oldIndex.getRolloverInfos().get("test_alias").getMetConditions(), is(empty()));


I think we should make an assertion on the getTime too. I see a way to do this: take the clock before you submit the request, then assert the value of getTime is more than that?

true, I guess I was trying avoid timing issues since the system time was being finicky. the move to threadPool makes things more consistent in their world view! thanks

jasontedor · 2018-06-13T01:47:43Z

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverIT.java

+            final IndexMetaData oldIndex = client().admin().cluster().prepareState().get().getState().metaData().index("test-1");
+            List<Condition> metConditions = oldIndex.getRolloverInfos().get("test_alias").getMetConditions();
+            assertThat(metConditions.size(), equalTo(1));
+            assertThat(metConditions.get(0).toString(), equalTo(new MaxSizeCondition(maxSizeValue).toString()));


Let us make an assertion on the clock too.

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

thanks for the round of review, I've addressed your comments and merged-latest-master/removed-unused-imports. thanks @jasontedor!

jasontedor

LGTM.

talevy · 2018-06-15T15:43:55Z

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

* 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 introduces a new property to IndexMetaData called RolloverInfo. This object contains a map containing the aliases that were used to rollover the related index, which conditions were met, and at what time the rollover took place. much like the `index.creation_date`, it captures the approximate time that the index was rolled over to a new one.

…31144) (#31413) This commit introduces a new property to IndexMetaData called RolloverInfo. This object contains a map containing the aliases that were used to rollover the related index, which conditions were met, and at what time the rollover took place. much like the `index.creation_date`, it captures the approximate time that the index was rolled over to a new one. set version serialization check to 6.4

* 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)

Add rollover-creation-date setting to rolled over index

7817a9f

This PR introduces a new index setting `index.rollover.creation_date` much like the `index.creation_date`, it captures the approximate time that the index was rolled over to a new one.

talevy added >enhancement review :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. labels Jun 6, 2018

talevy requested a review from colings86 June 6, 2018 16:24

jasontedor reviewed Jun 7, 2018

View reviewed changes

talevy added 4 commits June 7, 2018 12:12

Revert "Add rollover-creation-date setting to rolled over index"

c009112

This reverts commit 7817a9f.

Merge branch 'master' into rollover-date

e42c86c

update to place RolloverInfo in IndexMetaData

3c4dea6

This commit introduces a new property to IndexMetaData called RolloverInfo. This object contains a map containing the aliases that were used to rollover the related index, which conditions were met, and at what time the rollover took place.

Merge branch 'master' into rollover-date

6b441f2

talevy commented Jun 11, 2018

View reviewed changes

talevy commented Jun 12, 2018

View reviewed changes

talevy added 5 commits June 11, 2018 21:22

Merge branch 'master' into rollover-date

8d5b6ec

fix line-length

a1d6786

Merge branch 'master' into rollover-date

35e773e

update serialization

59d66ce

cleanup fromXContent

1bde84d

jasontedor reviewed Jun 13, 2018

View reviewed changes

talevy added 7 commits June 12, 2018 21:43

Merge branch 'master' into rollover-date

e8fc71d

respond to review and add test for rollover time

b96b897

Merge branch 'master' into rollover-date

bced90b

make time check more lenient by a second

670468e

Merge branch 'master' into rollover-date

d014b6e

Merge branch 'master' into rollover-date

53ef36f

remove unused imports

7b9a25d

jasontedor approved these changes Jun 15, 2018

View reviewed changes

talevy merged commit eda4964 into elastic:master Jun 15, 2018

talevy deleted the rollover-date branch June 15, 2018 15:44

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

talevy mentioned this pull request Jun 18, 2018

lower rollover-info version bound to 6.4 #31414

Merged

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

Conversation

talevy commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 6, 2018

Uh oh!

talevy commented Jun 6, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

talevy commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy commented Jun 14, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

talevy commented Jun 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

talevy commented Jun 6, 2018 •

edited

Loading

talevy commented Jun 12, 2018 •

edited

Loading