-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow indices lookup to be built lazily #78745
Changes from 2 commits
a6cbfce
0ea5c7e
e91f474
3d816a0
8c529e7
e2e31a4
0c81c4a
6b45524
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,7 +189,7 @@ public interface NonRestorableCustom extends Custom { | |
private final String[] allClosedIndices; | ||
private final String[] visibleClosedIndices; | ||
|
||
private final SortedMap<String, IndexAbstraction> indicesLookup; | ||
private volatile SortedMap<String, IndexAbstraction> indicesLookup; | ||
|
||
Metadata(String clusterUUID, boolean clusterUUIDCommitted, long version, CoordinationMetadata coordinationMetadata, | ||
Settings transientSettings, Settings persistentSettings, DiffableStringMap hashesOfConsistentSettings, | ||
|
@@ -304,6 +304,13 @@ public boolean equalsAliases(Metadata other) { | |
} | ||
|
||
public SortedMap<String, IndexAbstraction> getIndicesLookup() { | ||
if (indicesLookup != null) { | ||
return indicesLookup; | ||
} | ||
synchronized (this) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, let's not have any synchronization here. If this isn't set then it will be lazy set just fine in our task loops and otherwise it's set from the time of construction anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
DataStreamMetadata dataStreamMetadata = custom(DataStreamMetadata.TYPE); | ||
indicesLookup = Collections.unmodifiableSortedMap(Builder.buildIndicesLookup(dataStreamMetadata, indices)); | ||
} | ||
return indicesLookup; | ||
} | ||
|
||
|
@@ -1431,6 +1438,10 @@ public Builder generateClusterUuidIfNeeded() { | |
} | ||
|
||
public Metadata build() { | ||
return build(true); | ||
} | ||
|
||
public Metadata build(boolean builtIndicesLookupEagerly) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this one deserves a little Javadoc :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// TODO: We should move these datastructures to IndexNameExpressionResolver, this will give the following benefits: | ||
// 1) The datastructures will be rebuilt only when needed. Now during serializing we rebuild these datastructures | ||
// while these datastructures aren't even used. | ||
|
@@ -1514,9 +1525,15 @@ public Metadata build() { | |
"were found [" + Strings.collectionToCommaDelimitedString(duplicates) + "]"); | ||
} | ||
|
||
SortedMap<String, IndexAbstraction> indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); | ||
ImmutableOpenMap<String, IndexMetadata> indices = this.indices.build(); | ||
|
||
SortedMap<String, IndexAbstraction> indicesLookup; | ||
if (builtIndicesLookupEagerly) { | ||
indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup(dataStreamMetadata, indices)); | ||
} else { | ||
indicesLookup = null; | ||
} | ||
|
||
validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); | ||
|
||
// build all concrete indices arrays: | ||
// TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices. | ||
|
@@ -1530,14 +1547,14 @@ public Metadata build() { | |
String[] visibleClosedIndicesArray = visibleClosedIndices.toArray(Strings.EMPTY_ARRAY); | ||
|
||
return new Metadata(clusterUUID, clusterUUIDCommitted, version, coordinationMetadata, transientSettings, persistentSettings, | ||
hashesOfConsistentSettings, indices.build(), templates.build(), customs.build(), allIndicesArray, visibleIndicesArray, | ||
hashesOfConsistentSettings, indices, templates.build(), customs.build(), allIndicesArray, visibleIndicesArray, | ||
allOpenIndicesArray, visibleOpenIndicesArray, allClosedIndicesArray, visibleClosedIndicesArray, indicesLookup); | ||
} | ||
|
||
private SortedMap<String, IndexAbstraction> buildIndicesLookup() { | ||
static SortedMap<String, IndexAbstraction> buildIndicesLookup(DataStreamMetadata dataStreamMetadata, | ||
ImmutableOpenMap<String, IndexMetadata> indices) { | ||
SortedMap<String, IndexAbstraction> indicesLookup = new TreeMap<>(); | ||
Map<String, DataStream> indexToDataStreamLookup = new HashMap<>(); | ||
DataStreamMetadata dataStreamMetadata = (DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE); | ||
// If there are no indices, then skip data streams. This happens only when metadata is read from disk | ||
if (dataStreamMetadata != null && indices.size() > 0) { | ||
Map<String, List<String>> dataStreamToAliasLookup = new HashMap<>(); | ||
|
@@ -1579,9 +1596,7 @@ private SortedMap<String, IndexAbstraction> buildIndicesLookup() { | |
} | ||
|
||
Map<String, List<IndexMetadata>> aliasToIndices = new HashMap<>(); | ||
for (var cursor : indices.values()) { | ||
IndexMetadata indexMetadata = cursor.value; | ||
|
||
for (var indexMetadata : indices.values()) { | ||
IndexAbstraction.Index index; | ||
DataStream parent = indexToDataStreamLookup.get(indexMetadata.getIndex().getName()); | ||
if (parent != null) { | ||
|
@@ -1600,12 +1615,12 @@ private SortedMap<String, IndexAbstraction> buildIndicesLookup() { | |
|
||
for (var aliasCursor : indexMetadata.getAliases()) { | ||
AliasMetadata aliasMetadata = aliasCursor.value; | ||
aliasToIndices.compute(aliasMetadata.getAlias(), (aliasName, indices) -> { | ||
if (indices == null) { | ||
indices = new ArrayList<>(); | ||
aliasToIndices.compute(aliasMetadata.getAlias(), (aliasName, val) -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're changing this already (+1 to the name change btw), can't we just go for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if (val == null) { | ||
val = new ArrayList<>(); | ||
} | ||
indices.add(indexMetadata); | ||
return indices; | ||
val.add(indexMetadata); | ||
return val; | ||
}); | ||
} | ||
} | ||
|
@@ -1616,6 +1631,7 @@ private SortedMap<String, IndexAbstraction> buildIndicesLookup() { | |
assert existing == null : "duplicate for " + entry.getKey(); | ||
} | ||
|
||
validateDataStreams(indicesLookup, dataStreamMetadata); | ||
return indicesLookup; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this to be
volatile
, this is just a best effort cache. No need to synchronize here. Especially when the only time this isnull
after construction is during the ILM task loop anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3d816a0