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
Simplify ConcreteIndices and its usage in TransportBulkAction #81098
Conversation
@@ -591,7 +591,7 @@ public void onResponse(BulkShardResponse bulkShardResponse) { | |||
public void onFailure(Exception e) { | |||
// create failures for all relevant requests | |||
for (BulkItemRequest request : requests) { | |||
final String indexName = concreteIndices.getConcreteIndex(request.index()).getName(); | |||
final String indexName = request.index(); |
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 to lookup Index
from ConcreteIndices
if we just use it as name in an exception message.
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.
I suspect that this one line could give some pretty big memory savings on the coordinating node sometimes. Prior to this change we'd retain a reference to the ConcreteIndices
(and therefore the ClusterState
) while the request was ongoing. Now we don't. Great stuff. 🎉
IndexNotFoundException cannotCreate = indicesThatCannotBeCreated.get(request.index()); | ||
if (cannotCreate != null) { | ||
addFailure(request, idx, cannotCreate); | ||
return true; | ||
} | ||
Index concreteIndex = concreteIndices.getConcreteIndex(request.index()); |
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.
The ConcreteIndices#resolveIfAbsent(...)
already checks whether an Index
instance has been cached, so
no need to this here as well.
@@ -738,10 +727,6 @@ void executeBulk( | |||
this.indexNameExpressionResolver = indexNameExpressionResolver; | |||
} | |||
|
|||
Index getConcreteIndex(String indexOrAlias) { |
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.
After removing the above two usages, this method can be removed as it is no longer used.
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.
LGTM!
* upstream/master: [ML] Parent datafeed actions to the datafeed's persistent task (elastic#81143) Simplify ConcreteIndices and its usage in TransportBulkAction (elastic#81098) Unmute DataStreamsSnapshotsIT#testRestoreDataStreamAliasWithConflictingIndicesAlias() test (elastic#81142) TSDB: Do not allow index splits for time series indices (elastic#81125) Reduce verbosity-increase timeout to 3m (elastic#81118) Mute DataStreamsSnapshotsIT#testRestoreDataStreamAliasWithConflictingIndicesAlias() test Fix stopping of old elasticsearch cluster (elastic#81059) Fix data stream alias validation. (elastic#81040) Add replicated field to get data stream api response. (elastic#80988)
No description provided.