Create first backing index when creating data stream #54467

danhermann · 2020-03-30T21:09:25Z

Eagerly creates the first backing index when a data stream is created.

Relates to #53100

elasticmachine · 2020-03-30T21:09:27Z

Pinging @elastic/es-core-features (:Core/Features/Data streams)

henningandersen

Thanks @danhermann , I left a few comments to consider, but looking good otherwise.

henningandersen · 2020-03-31T06:28:34Z

.../src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java

            if (currentState.metaData().dataStreams().containsKey(request.name)) {
                throw new IllegalArgumentException("data_stream [" + request.name + "] already exists");
            }

            MetaDataCreateIndexService.validateIndexOrAliasName(request.name,
                (s1, s2) -> new IllegalArgumentException("data_stream [" + s1 + "] " + s2));

+            String firstBackingIndexName = request.name + "-000000";


I think I remember @dakrone mentioning something about not starting from 0, rather from 1 like described in the ILM getting started guide. I am OK either way.

Good point. I think we should be consistent with what ILM does.

henningandersen · 2020-03-31T06:33:54Z

.../src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java

+                new CreateIndexClusterStateUpdateRequest("initialize_data_stream", firstBackingIndexName, firstBackingIndexName);
+            currentState = metaDataCreateIndexService.applyCreateIndexRequest(currentState, createIndexRequest, false);
+            IndexMetaData firstBackingIndex = currentState.metaData().index(firstBackingIndexName);
+


It would be good to add:

Suggested change

assert firstBackingIndex != null;

since that is guaranteed to fail tests (the NPE occurring further down could be swallowed).

henningandersen · 2020-03-31T06:47:32Z

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

@@ -250,4 +252,42 @@ private boolean isNonEmpty(List<IndexMetaData> idxMetas) {
            return (Objects.isNull(idxMetas) || idxMetas.isEmpty()) == false;
        }
    }
+
+    class DataStream implements IndexAbstraction {


This looks unused? I think it makes more sense to add this when also building the indicesLookup.

henningandersen · 2020-03-31T06:48:52Z

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

@@ -1377,12 +1379,27 @@ private void validateDataStreams(SortedMap<String, IndexAbstraction> indicesLook
            DataStreamMetadata dsMetadata = (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE);
            if (dsMetadata != null) {
                for (DataStream ds : dsMetadata.dataStreams().values()) {
-                    if (indicesLookup.containsKey(ds.getName())) {
+                    IndexAbstraction existing = indicesLookup.get(ds.getName());
+                    if (existing != null && existing.getType() != IndexAbstraction.Type.DATA_STREAM) {


We should end up asserting that existing != null, but that can be done in a followup together with populating indicesLookup

henningandersen · 2020-03-31T06:53:27Z

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

                    if (map.size() != 0) {
+                        if (map.size() == ds.getIndices().size()) {
+                            int numValidIndices = 0;
+                            for (int i = 0; i < map.size(); i++) {


I think this should instead validate that the entries in map are the same as those in ds.getIndices().

Right now it works in the initial create case, but not after we have deleted the first index.

Also it would be good to add a MetaDataTests test that validates that a data-stream pointing to backing indices (with random suffix number) works.

martijnvg

I left a few comments.

martijnvg · 2020-03-31T06:54:55Z

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

                        throw new IllegalStateException("data stream [" + ds.getName() + "] conflicts with existing index or alias");
                    }

-                    SortedMap<?, ?> map = indicesLookup.subMap(ds.getName() + "-", ds.getName() + "."); // '.' is the char after '-'
+                    SortedMap<String, IndexAbstraction> map =


Let's add this in another change as well.

Never mind this comment. This change is necessary otherwise the first backing index of data stream can't be created.

.../src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java

martijnvg · 2020-03-31T07:00:14Z

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

@@ -250,4 +252,42 @@ private boolean isNonEmpty(List<IndexMetaData> idxMetas) {
            return (Objects.isNull(idxMetas) || idxMetas.isEmpty()) == false;
        }
    }
+
+    class DataStream implements IndexAbstraction {


Let's add the data stream index abstraction in another change? We can use the get index api in the yaml test to check that the index has correctly been created.

(the logic that adds data streams to the indicesLookup is missing here, which would need to be added too, but let's try to this pr small, so that it is easy to review)
(also a number of if statements in the code base may need to be revised because a new data stream type is introduced)

martijnvg

Left some minor comments, LGTM otherwise.

martijnvg · 2020-04-01T06:48:42Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml

@@ -22,10 +22,15 @@
      indices.get_data_streams: {}
  - match: { 0.name: simple-data-stream1 }
  - match: { 0.timestamp_field: '@timestamp' }
-  - match: { 0.indices: [] }
+  - length: { 0.indices: 1 }


Can also the index name be checked here inside the indices array field?

@martijnvg, it looks like the format of the backing indices response changed when the data stream's indices member was changed from List<String> to List<Index>. It now looks like this:

{ "name" : "simple-data-stream1", "timestamp_field" : "@timestamp", "indices" : [ { "index_name" : "simple-data-stream1-000001", "index_uuid" : "V3ONJviCQB68b1BBzMJwMw" } ] }

Is that ok?

Yes, this is intended. We can add this assert:

- match: {0.indices.0.index_name: 'simple-data-stream1-000001')

never mind my comment, you already made this change :)

martijnvg · 2020-04-01T06:52:39Z

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

-        ALIAS("alias");
+        ALIAS("alias"),
+
+        DATA_STREAM("data_stream");


maybe add some java doc here?

martijnvg · 2020-04-01T09:17:18Z

...est/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java

        assertThat(e.getMessage(), containsString("must not contain the following characters"));
    }
+
+    private static class MockMetadataCreateIndexService extends MetadataCreateIndexService {


maybe instead of mock class use: Mockito#spy(...) and attach expected behaviour?

danhermann · 2020-04-01T18:36:26Z

@elasticmachine update branch

danhermann added >non-issue v8.0.0 :Data Management/Data streams Data streams and their lifecycles v7.8.0 labels Mar 30, 2020

danhermann requested review from martijnvg and henningandersen March 30, 2020 21:09

henningandersen reviewed Mar 31, 2020

View reviewed changes

martijnvg reviewed Mar 31, 2020

View reviewed changes

danhermann added 4 commits March 31, 2020 15:14

create first backing index

0ae87c5

update unit test

806acc4

temporarily restrict YML test

078dae1

wip on review comments

1284b2b

danhermann force-pushed the create_first_backing_index branch from 5055098 to 1284b2b Compare March 31, 2020 20:21

danhermann added 2 commits March 31, 2020 18:26

finish review comments and tests

df1bf05

fix test

47e6b88

martijnvg approved these changes Apr 1, 2020

View reviewed changes

danhermann added 2 commits April 1, 2020 12:20

wip on review comments

0bd4bd4

review comments

6c1856f

elasticmachine and others added 2 commits April 1, 2020 14:36

Merge branch 'master' into create_first_backing_index

fe2849d

fix test

9c9a42c

danhermann merged commit 42f513c into elastic:master Apr 2, 2020

danhermann added the backport pending label Apr 2, 2020

danhermann deleted the create_first_backing_index branch April 2, 2020 15:58

danhermann mentioned this pull request Apr 2, 2020

[7.x] Create first backing index when creating data stream (#54467) #54682

Merged

danhermann removed the backport pending label Apr 2, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create first backing index when creating data stream #54467

Create first backing index when creating data stream #54467

danhermann commented Mar 30, 2020

elasticmachine commented Mar 30, 2020

henningandersen left a comment

henningandersen Mar 31, 2020

danhermann Mar 31, 2020

henningandersen Mar 31, 2020

henningandersen Mar 31, 2020

henningandersen Mar 31, 2020

henningandersen Mar 31, 2020

martijnvg left a comment

martijnvg Mar 31, 2020

martijnvg Mar 31, 2020

martijnvg Mar 31, 2020

martijnvg left a comment

martijnvg Apr 1, 2020

danhermann Apr 1, 2020

martijnvg Apr 2, 2020

martijnvg Apr 2, 2020

martijnvg Apr 1, 2020

martijnvg Apr 1, 2020

danhermann commented Apr 1, 2020

Create first backing index when creating data stream #54467

Create first backing index when creating data stream #54467

Conversation

danhermann commented Mar 30, 2020

elasticmachine commented Mar 30, 2020

henningandersen 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

martijnvg 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

martijnvg 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

danhermann commented Apr 1, 2020