Refactor IndexAbstraction to not use IndexMetadata #79080
Conversation
Most users of an `IndexAbstraction` instance doesn't need to use the `IndexMetadata` instances that `getIndices()` and `getWriteIndex()` returns. Cluster state variables/parameters can be used in places that access to `IndexMetadata` is required. By changing the `getIndices()` and `getWriteIndex()` methods to return `Index` instance, the indices lookup can be reused across different cluster states. This should be possible in cases that don't change an index hidden status or open and closes indices or when adding / removing aliases, data streams or indices. This change should allow for elastic#79004
martijnvg
added
:Data Management/Indices APIs
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
original-brownbear
left a comment
original-brownbear
left a comment
There was a problem hiding this comment.
Thanks @martijnvg this looks really good. I just have a couple of smaller suggestions/questions on performance. Maybe they're irrelevant but I'm paranoid now :)
| String concreteIndex = index.getName(); | ||
| AliasMetadata aliasMetadata = state.metadata().index(concreteIndex).getAliases().get(indexAbstraction.getName()); | ||
| if (norouting.contains(concreteIndex) == false) { | ||
| if (aliasMetadata != null && aliasMetadata.searchRoutingValues().isEmpty() == false) { |
There was a problem hiding this comment.
I think we can move:
AliasMetadata aliasMetadata = state.metadata().index(concreteIndex).getAliases().get(indexAbstraction.getName());
down here right to not do the newly added map lookup more often than necessary?
| } else if (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM && context.isResolveToWriteIndex()) { | ||
| IndexMetadata writeIndex = indexAbstraction.getWriteIndex(); | ||
| IndexMetadata writeIndex = metadata.index(indexAbstraction.getWriteIndex()); | ||
| if (addIndex(writeIndex, context)) { |
There was a problem hiding this comment.
I wonder if we should make addIndex just accept Metadata and Index instead of IndexMetadata, then we can do without the additional map lookup altogether except for when we resolve with ignored_throttled==true?
| this.dataStream = dataStream; | ||
| this.dataStreamIndices = List.copyOf(dataStreamIndices); | ||
| this.writeIndex = dataStreamIndices.get(dataStreamIndices.size() - 1); | ||
| this.dataStreamIndices = dataStreamIndices.stream() |
There was a problem hiding this comment.
Performance does matter a little here if I remember correctly (might just be our benchmarks with a ton of DS though). Might be worth it to just pre-size an array here and copy or wrap that or to adjust the constructor outright because we might be able to just pass the Index list here to begin with?
There was a problem hiding this comment.
I think we can just pass down a List here. The place where this gets constructed can just create a List instead of List.
| this.aliasName = dataStreamAlias.getName(); | ||
| this.referenceIndexMetadatas = indicesOfAllDataStreams; | ||
| this.writeIndex = writeIndexOfWriteDataStream; | ||
| this.referenceIndexMetadatas = indicesOfAllDataStreams.stream() |
There was a problem hiding this comment.
Same reservations as with the DS class, we should be careful not to alter the performance characteristics of this too much shouldn't we?
|
@elasticmachine run elasticsearch-ci/bwc |
original-brownbear
left a comment
original-brownbear
left a comment
There was a problem hiding this comment.
LGTM, thanks for the iterations @martijnvg !
|
@elasticmachine run elasticsearch-ci/bwc (unrelated failure that reproduces on master also) |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/part-1 |
Backport of elastic#79080 to 7.x branch. Most users of an `IndexAbstraction` instance doesn't need to use the `IndexMetadata` instances that `getIndices()` and `getWriteIndex()` returns. Cluster state variables/parameters can be used in places that access to `IndexMetadata` is required. By changing the `getIndices()` and `getWriteIndex()` methods to return `Index` instance, the indices lookup can be reused across different cluster states. This should be possible in cases that don't change an index hidden status or open and closes indices or when adding / removing aliases, data streams or indices. This change should allow for elastic#79004
Backport of #79080 to 7.x branch. Most users of an `IndexAbstraction` instance doesn't need to use the `IndexMetadata` instances that `getIndices()` and `getWriteIndex()` returns. Cluster state variables/parameters can be used in places that access to `IndexMetadata` is required. By changing the `getIndices()` and `getWriteIndex()` methods to return `Index` instance, the indices lookup can be reused across different cluster states. This should be possible in cases that don't change an index hidden status or open and closes indices or when adding / removing aliases, data streams or indices. This change should allow for #79004
5 participants
Most users of an
IndexAbstractioninstance doesn't need to use theIndexMetadatainstances thatgetIndices()andgetWriteIndex()returns.Cluster state variables/parameters can be used in places that access to
IndexMetadatais required.By changing the
getIndices()andgetWriteIndex()methods to returnIndexinstance, the indices lookup can be reused across different cluster states.
This should be possible in cases that don't change an index hidden status or
open and closes indices or when adding / removing aliases, data streams or indices.
This change should allow for #79004