Skip to content

Commit

Permalink
Revert "Add ResolvedExpression wrapper (elastic#114592)" (elastic#115317
Browse files Browse the repository at this point in the history
) (elastic#115376)

This reverts commit 4c15cc0.
This commit introduced an orders of magnitude regression when searching many shards.

(cherry picked from commit d9baf6f)

Co-authored-by: Armin Braun <me@obrown.io>
  • Loading branch information
dakrone and original-brownbear authored Oct 23, 2024
1 parent 9f8edaf commit 10f711d
Show file tree
Hide file tree
Showing 16 changed files with 361 additions and 504 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/115317.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 115317
summary: Revert "Add `ResolvedExpression` wrapper"
area: Indices APIs
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import org.elasticsearch.cluster.block.ClusterBlockException;
import org.elasticsearch.cluster.block.ClusterBlockLevel;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.routing.GroupShardsIterator;
import org.elasticsearch.cluster.routing.ShardIterator;
Expand Down Expand Up @@ -85,7 +84,7 @@ protected void masterOperation(
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(clusterState, request);
Map<String, Set<String>> routingMap = indexNameExpressionResolver.resolveSearchRouting(state, request.routing(), request.indices());
Map<String, AliasFilter> indicesAndFilters = new HashMap<>();
Set<ResolvedExpression> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
for (String index : concreteIndices) {
final AliasFilter aliasFilter = indicesService.buildAliasFilter(clusterState, index, indicesAndAliases);
final String[] aliases = indexNameExpressionResolver.indexAliases(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -566,8 +565,8 @@ static void resolveIndices(
if (names.length == 1 && (Metadata.ALL.equals(names[0]) || Regex.isMatchAllPattern(names[0]))) {
names = new String[] { "**" };
}
Set<ResolvedExpression> resolvedIndexAbstractions = resolver.resolveExpressions(clusterState, indicesOptions, true, names);
for (ResolvedExpression s : resolvedIndexAbstractions) {
Set<String> resolvedIndexAbstractions = resolver.resolveExpressions(clusterState, indicesOptions, true, names);
for (String s : resolvedIndexAbstractions) {
enrichIndexAbstraction(clusterState, s, indices, aliases, dataStreams);
}
indices.sort(Comparator.comparing(ResolvedIndexAbstraction::getName));
Expand Down Expand Up @@ -598,12 +597,12 @@ private static void mergeResults(

private static void enrichIndexAbstraction(
ClusterState clusterState,
ResolvedExpression indexAbstraction,
String indexAbstraction,
List<ResolvedIndex> indices,
List<ResolvedAlias> aliases,
List<ResolvedDataStream> dataStreams
) {
IndexAbstraction ia = clusterState.metadata().getIndicesLookup().get(indexAbstraction.resource());
IndexAbstraction ia = clusterState.metadata().getIndicesLookup().get(indexAbstraction);
if (ia != null) {
switch (ia.getType()) {
case CONCRETE_INDEX -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.elasticsearch.cluster.block.ClusterBlockException;
import org.elasticsearch.cluster.block.ClusterBlockLevel;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.routing.GroupShardsIterator;
import org.elasticsearch.cluster.routing.ShardIterator;
import org.elasticsearch.cluster.routing.ShardRouting;
Expand Down Expand Up @@ -134,7 +133,7 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener
@Override
protected ShardValidateQueryRequest newShardRequest(int numShards, ShardRouting shard, ValidateQueryRequest request) {
final ClusterState clusterState = clusterService.state();
final Set<ResolvedExpression> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
final AliasFilter aliasFilter = searchService.buildAliasFilter(clusterState, shard.getIndexName(), indicesAndAliases);
return new ShardValidateQueryRequest(shard.shardId(), aliasFilter, request);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.elasticsearch.action.support.single.shard.TransportSingleShardAction;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.routing.ShardIterator;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.io.stream.Writeable;
Expand Down Expand Up @@ -110,7 +109,7 @@ protected boolean resolveIndex(ExplainRequest request) {

@Override
protected void resolveRequest(ClusterState state, InternalRequest request) {
final Set<ResolvedExpression> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(state, request.request().index());
final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(state, request.request().index());
final AliasFilter aliasFilter = searchService.buildAliasFilter(state, request.concreteIndex(), indicesAndAliases);
request.request().filteringAlias(aliasFilter);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.routing.GroupShardsIterator;
Expand Down Expand Up @@ -111,7 +110,6 @@
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.LongSupplier;
import java.util.stream.Collectors;

import static org.elasticsearch.action.search.SearchType.DFS_QUERY_THEN_FETCH;
import static org.elasticsearch.action.search.SearchType.QUERY_THEN_FETCH;
Expand Down Expand Up @@ -205,17 +203,14 @@ public TransportSearchAction(

private Map<String, OriginalIndices> buildPerIndexOriginalIndices(
ClusterState clusterState,
Set<ResolvedExpression> indicesAndAliases,
Set<String> indicesAndAliases,
String[] indices,
IndicesOptions indicesOptions
) {
Map<String, OriginalIndices> res = Maps.newMapWithExpectedSize(indices.length);
var blocks = clusterState.blocks();
// optimization: mostly we do not have any blocks so there's no point in the expensive per-index checking
boolean hasBlocks = blocks.global().isEmpty() == false || blocks.indices().isEmpty() == false;
// Get a distinct set of index abstraction names present from the resolved expressions to help with the reverse resolution from
// concrete index to the expression that produced it.
Set<String> indicesAndAliasesResources = indicesAndAliases.stream().map(ResolvedExpression::resource).collect(Collectors.toSet());
for (String index : indices) {
if (hasBlocks) {
blocks.indexBlockedRaiseException(ClusterBlockLevel.READ, index);
Expand All @@ -232,8 +227,8 @@ private Map<String, OriginalIndices> buildPerIndexOriginalIndices(
String[] finalIndices = Strings.EMPTY_ARRAY;
if (aliases == null
|| aliases.length == 0
|| indicesAndAliasesResources.contains(index)
|| hasDataStreamRef(clusterState, indicesAndAliasesResources, index)) {
|| indicesAndAliases.contains(index)
|| hasDataStreamRef(clusterState, indicesAndAliases, index)) {
finalIndices = new String[] { index };
}
if (aliases != null) {
Expand All @@ -252,11 +247,7 @@ private static boolean hasDataStreamRef(ClusterState clusterState, Set<String> i
return indicesAndAliases.contains(ret.getParentDataStream().getName());
}

Map<String, AliasFilter> buildIndexAliasFilters(
ClusterState clusterState,
Set<ResolvedExpression> indicesAndAliases,
Index[] concreteIndices
) {
Map<String, AliasFilter> buildIndexAliasFilters(ClusterState clusterState, Set<String> indicesAndAliases, Index[] concreteIndices) {
final Map<String, AliasFilter> aliasFilterMap = new HashMap<>();
for (Index index : concreteIndices) {
clusterState.blocks().indexBlockedRaiseException(ClusterBlockLevel.READ, index.getName());
Expand Down Expand Up @@ -1246,10 +1237,7 @@ private void executeSearch(
} else {
final Index[] indices = resolvedIndices.getConcreteLocalIndices();
concreteLocalIndices = Arrays.stream(indices).map(Index::getName).toArray(String[]::new);
final Set<ResolvedExpression> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(
clusterState,
searchRequest.indices()
);
final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, searchRequest.indices());
aliasFilter = buildIndexAliasFilters(clusterState, indicesAndAliases, indices);
aliasFilter.putAll(remoteAliasMap);
localShardIterators = getLocalShardsIterator(
Expand Down Expand Up @@ -1822,7 +1810,7 @@ List<SearchShardIterator> getLocalShardsIterator(
ClusterState clusterState,
SearchRequest searchRequest,
String clusterAlias,
Set<ResolvedExpression> indicesAndAliases,
Set<String> indicesAndAliases,
String[] concreteIndices
) {
var routingMap = indexNameExpressionResolver.resolveSearchRouting(clusterState, searchRequest.routing(), searchRequest.indices());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import org.elasticsearch.action.support.HandledTransportAction;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression;
import org.elasticsearch.cluster.routing.GroupShardsIterator;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.index.Index;
Expand Down Expand Up @@ -128,10 +127,7 @@ public void searchShards(Task task, SearchShardsRequest searchShardsRequest, Act
searchService.getRewriteContext(timeProvider::absoluteStartMillis, resolvedIndices, null),
listener.delegateFailureAndWrap((delegate, searchRequest) -> {
Index[] concreteIndices = resolvedIndices.getConcreteLocalIndices();
final Set<ResolvedExpression> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(
clusterState,
searchRequest.indices()
);
final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, searchRequest.indices());
final Map<String, AliasFilter> aliasFilters = transportSearchAction.buildIndexAliasFilters(
clusterState,
indicesAndAliases,
Expand Down
Loading

0 comments on commit 10f711d

Please sign in to comment.