Refactor AliasOrIndex abstraction. #53982

martijnvg · 2020-03-23T14:27:59Z

In order to prepare the AliasOrIndex abstraction for the introduction of data streams,
the abstraction needs to be made more flexible, because currently it really can be only
an alias or an index.

The following changes were made:

Renamed AliasOrIndex to IndexAbstraction.
Introduced a IndexSpace.Type enum to indicate what a IndexSpace instance is.
Replaced the isAlias() method that returns a boolean with the getType() method that returns the new Type enum.
Moved getWriteIndex() up from the IndexSpace.Alias to the IndexSpace interface.
Moved getAliasName() up from the IndexSpace.Alias to the IndexSpace interface and renamed it to getName().
Removed unnecessary casting to IndexSpace.Alias by just checking the getType() method.

Relates to #53100

In order to prepare the `AliasOrIndex` abstraction for the introduction of data streams, the abstraction needs to be made more flexible, because currently it really can be only an alias or an index. * Introduced a `AliasOrIndex.Type` enum to indicate what a `AliasOrIndex` instance is. * Replaced the `isAlias()`` method that returns a boolean with the `getType()`` method that returns the new Type enum. * Moved `getWriteIndex()`` up from the `AliasOrIndex.Alias` to the `AliasOrIndex` interface. * Moved `g`etAliasName()`` up from the `AliasOrIndex.Alias` to the `AliasOrIndex` interface and renamed it to `getName()``. * Removed unnecessary casting to `AliasOrIndex.Alias` by just checking the `getType()`` method. Finally `AliasOrIndex` should be renamed to reflect that it can be more than just an index or alias, since in the near future it can also be a data stream. The name AliasOrIndexOrDataStream is not appealing to me. We can rename it to `Namespace`, but that sounds to generic to me. `ResolvedIndicesExpression` sounds better to me, since it reflects more what it is (an expression from api that has been resolved to alias/index/datasteam), but the name itself is a bit on the long side. Relates to elastic#53100

elasticmachine · 2020-03-23T14:28:01Z

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

…abstraction

danhermann · 2020-03-25T15:09:19Z

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

        }
-        if (aliasOrIndex.isAlias() == false) {
+        if (indexSpace.getType() != IndexSpace.Type.ALIAS) {
            throw new IllegalArgumentException("source alias is a concrete index");


Looks like a lot of error messages such as this one that assume only alias or index types would benefit from wording changes that accommodate data streams, too.

Good point. I will go over these checks and messages and adjust.

danhermann · 2020-03-25T15:14:33Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

-                if (indexOrAlias != null && indexOrAlias.isAlias()) {
-                    AliasOrIndex.Alias alias = (AliasOrIndex.Alias) indexOrAlias;
-                    indexMetaData = alias.getWriteIndex();
+                IndexSpace indexOrAlias = metaData.getAliasAndIndexLookup().get(indexRequest.index());


We'll probably want to rename that variable from indexOrAlias. Can be done in a later PR, though.

danhermann · 2020-03-25T15:16:45Z

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

-     * @return whether this alias/index is hidden or not
+     * A write index is a dedicated concrete index, that accepts all the new documents that belong to an index space.
+     *
+     * A write index may also be a regular concrete index of a index space and may be therefor also be returned


nit: "may be therefor also be returned" -> "may therefore also be returned"

danhermann · 2020-03-25T15:24:37Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java

 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.cluster.metadata.AliasMetaData;
-import org.elasticsearch.cluster.metadata.AliasOrIndex;
+import org.elasticsearch.cluster.metadata.IndexSpace;


Will we want to eventually rename this class to something like IndexSpaceResolver?

Yes, I think so.

danhermann

LGTM. I left a few comments that are minor, not actionable, and/or addressable in later PRs.

…abstraction

renamed `MetaData#aliasAndIndexLookup` field (and getter) to `MetaData#indexSpaceLookup`.

henningandersen · 2020-03-26T11:13:32Z

I wonder if we should use AbstractIndex instead of IndexSpace? It could be good to come up with a terminology for alias/index/datastream that we would consider using in documentation too.

danhermann · 2020-03-26T12:55:55Z

I wonder if we should use AbstractIndex instead of IndexSpace? It could be good to come up with a terminology for alias/index/datastream that we would consider using in documentation too.

We considered a bunch of different options including AliasOrIndexOrDataStream (not really), Namespace, ResolvedIndexExpression, and IndexFamily and landed on IndexSpace as the best of them. I agree that it would be good to have one that works well in documentation, too. Naming things is hard.

martijnvg · 2020-03-26T14:49:58Z

Dan and I did came up with the following alternative: IndexAbstraction.

AbstractIndex is very 'programming' term and could indicate to someone reading that name, that it isn't concrete. Whereas IndexAbstraction reads as an abstraction of an index, which represents something and which could be a concrete index, an alias or a data stream.

…abstraction

and fixed test

…duced.

henningandersen

LGTM.

I left a few smaller comments to consider.

henningandersen · 2020-03-27T11:06:58Z

...orization-engine/src/test/java/org/elasticsearch/example/CustomAuthorizationEngineTests.java

        CustomAuthorizationEngine engine = new CustomAuthorizationEngine();
-        Map<String, AliasOrIndex> aliasOrIndexMap = new HashMap<>();
-        aliasOrIndexMap.put("index", new Index(IndexMetaData.builder("index")
+        Map<String, IndexAbstraction> indexSpaceMap = new HashMap<>();


Remove "Space" from variable name?

henningandersen · 2020-03-27T11:32:42Z

.../test/java/org/elasticsearch/action/admin/indices/rollover/MetaDataRolloverServiceTests.java

            assertThat(rolloverIndexMetaData.getNumberOfShards(), equalTo(numberOfShards));

-            AliasOrIndex.Alias alias = (AliasOrIndex.Alias) rolloverMetaData.getAliasAndIndexLookup().get(aliasName);
+            IndexAbstraction alias = rolloverMetaData.getIndicesLookup().get(aliasName);


nit: also verify type (since we no longer do the cast).

henningandersen · 2020-03-27T12:00:41Z

.../test/java/org/elasticsearch/action/admin/indices/rollover/MetaDataRolloverServiceTests.java

        exception = expectThrows(IllegalArgumentException.class, () ->
            MetaDataRolloverService.validate(metaData, randomFrom(index1, index2)));
-        assertThat(exception.getMessage(), equalTo("source alias is a concrete index"));
+        assertThat(exception.getMessage(), equalTo("source alias is not an alias"));


I think the change to the exception message is not really a big issue. But with data streams also appearing, it could be nice to stick to the old message since that would give you some information on whether it is a datastream or concrete index. I think we could easily generate such a message by adding a display name ("concrete index") to the IndexAbstract.Type enum?

Also, this does make me wonder if Type.INDEX should be Type.CONCRETE_INDEX for consistency with that display name? Not truly important to me, feel free to ignore this part.

henningandersen · 2020-03-27T12:05:03Z

...ity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java

-        } else if (aliasInfo.isAlias() == false) {
-            logger.warn("service provider index [{}] exists as a concrete index, but it should be an alias", ALIAS_NAME);
+        } else if (aliasInfo.getType() != IndexAbstraction.Type.ALIAS) {
+            logger.warn("service provider index [{}] does not exist as an alias, but it should be", ALIAS_NAME);


Same comment on retaining the original message here.

henningandersen · 2020-03-27T12:06:13Z

x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/MlConfigMigratorIT.java

        MetaData metaData = mock(MetaData.class);
-        SortedMap<String, AliasOrIndex> aliasOrIndexSortedMap = new TreeMap<>();
-        when(metaData.getAliasAndIndexLookup()).thenReturn(aliasOrIndexSortedMap);
+        SortedMap<String, IndexAbstraction> indexSpaceSortedMap = new TreeMap<>();


Remove "Space" from variable name?

henningandersen · 2020-03-27T12:07:21Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java

-        } else if (aliasOrIndex.isAlias()) {
-            throw new IllegalStateException("concrete index [" + concreteIndexName + "] is an alias but should not be");
+        } else if (indexAbstraction.getType() != IndexAbstraction.Type.INDEX) {
+            throw new IllegalStateException("concrete index [" + concreteIndexName + "] is not an index but should be");


I think we should also include the display name of the actual type here.

…abstraction

In order to prepare the `AliasOrIndex` abstraction for the introduction of data streams, the abstraction needs to be made more flexible, because currently it really can be only an alias or an index. * Renamed `AliasOrIndex` to `IndexAbstraction`. * Introduced a `IndexAbstraction.Type` enum to indicate what a `IndexAbstraction` instance is. * Replaced the `isAlias()` method that returns a boolean with the `getType()` method that returns the new Type enum. * Moved `getWriteIndex()` up from the `IndexAbstraction.Alias` to the `IndexAbstraction` interface. * Moved `getAliasName()` up from the `IndexAbstraction.Alias` to the `IndexAbstraction` interface and renamed it to `getName()`. * Removed unnecessary casting to `IndexAbstraction.Alias` by just checking the `getType()` method. Relates to elastic#53100

Backport of #53982 In order to prepare the `AliasOrIndex` abstraction for the introduction of data streams, the abstraction needs to be made more flexible, because currently it really can be only an alias or an index. * Renamed `AliasOrIndex` to `IndexAbstraction`. * Introduced a `IndexAbstraction.Type` enum to indicate what a `IndexAbstraction` instance is. * Replaced the `isAlias()` method that returns a boolean with the `getType()` method that returns the new Type enum. * Moved `getWriteIndex()` up from the `IndexAbstraction.Alias` to the `IndexAbstraction` interface. * Moved `getAliasName()` up from the `IndexAbstraction.Alias` to the `IndexAbstraction` interface and renamed it to `getName()`. * Removed unnecessary casting to `IndexAbstraction.Alias` by just checking the `getType()` method. Relates to #53100

martijnvg added >non-issue :Data Management/Data streams Data streams and their lifecycles labels Mar 23, 2020

martijnvg added :Data Management/Indices APIs APIs to create and manage indices and templates v7.7.0 v8.0.0 and removed :Data Management/Data streams Data streams and their lifecycles labels Mar 23, 2020

martijnvg added 5 commits March 23, 2020 16:02

fixed checkstyle violation

fdae62b

tweaked logic

7a22b19

Merge remote-tracking branch 'es/master' into improve_alias_or_index_…

3908681

…abstraction

fixed compile error after merging in master

6d4eb8e

renamed AliasOrIndex to IndexSpace and added jdocs

a31b65d

martijnvg requested review from danhermann and henningandersen March 25, 2020 08:02

martijnvg marked this pull request as ready for review March 25, 2020 08:02

Merge remote-tracking branch 'es/master' into improve_alias_or_index_…

146535b

…abstraction

bpintea added v7.8.0 and removed v7.7.0 labels Mar 25, 2020

danhermann reviewed Mar 25, 2020

View reviewed changes

danhermann approved these changes Mar 25, 2020

View reviewed changes

martijnvg added 2 commits March 26, 2020 09:58

Merge remote-tracking branch 'es/master' into improve_alias_or_index_…

d6c053c

…abstraction

applied review comments and

48084dd

renamed `MetaData#aliasAndIndexLookup` field (and getter) to `MetaData#indexSpaceLookup`.

Merge remote-tracking branch 'es/master' into improve_alias_or_index_…

c81a77f

…abstraction

martijnvg added 4 commits March 27, 2020 10:21

changed MetDataRolloverService to use IndexSpace after merge

ae07254

and fixed test

renamed IndexSpace to IndexAbstraction

ea803a2

tweaked checks to also work if more index abstraction types are intro…

cabb45e

…duced.

List.of(...) everywhere

5c47b68

henningandersen approved these changes Mar 27, 2020

View reviewed changes

martijnvg added 4 commits March 27, 2020 14:04

Merge remote-tracking branch 'es/master' into improve_alias_or_index_…

57ffeaf

…abstraction

iter

f6c9e97

fixed checkstyle violation

2252334

fixed checkstyle violation (2)

90f6a2d

martijnvg merged commit b7af852 into elastic:master Mar 30, 2020

martijnvg added the backport pending label Mar 30, 2020

martijnvg mentioned this pull request Mar 30, 2020

Refactor AliasOrIndex abstraction. #54394

Merged

martijnvg removed the backport pending label Mar 30, 2020

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

Refactor AliasOrIndex abstraction. #53982

Refactor AliasOrIndex abstraction. #53982

Uh oh!

Conversation

martijnvg commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 23, 2020

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!

danhermann left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen commented Mar 26, 2020

Uh oh!

danhermann commented Mar 26, 2020

Uh oh!

martijnvg commented Mar 26, 2020

Uh oh!

henningandersen 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

martijnvg commented Mar 23, 2020 •

edited

Loading