Skip to content
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

Parse alias filters on the coordinating node #20916

Merged
merged 10 commits into from Oct 14, 2016

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Oct 13, 2016

Today we don't parse alias filters on the coordinating node, we only forward
the alias patters to executing node and resolve it late. This has several problems
like requests that go through filtered aliases are never cached if they use date math,
since the parsing happens very late in the process even without rewriting. It also used
to be processed on every shard while we can only do it once per index on the coordinating node.
Another nice side effect is that we are never prone to cluster-state updates that change an alias,
all nodes will execute the exact same alias filter since they are process based on the same
cluster state.

Parse alias filters on the coordinating node
Today we don't parse alias filters on the coordinating node, we only forward
the alias patters to executing node and resolve it late. This has several problems
like requests that go through filtered aliases are never cached if they use date math,
since the parsing happens very late in the process even without rewriting. It also used
to be processed on every shard while we can only do it once per index on the coordinating node.
Another nice sideeffect is that we are never prone to clusterstate updates that change an alias,
all nodes will execute the exact same alias filter since they are process based on the same
cluster state.
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2016

@colings86 can you take a look

@colings86
Copy link
Member

left a comment

Left one tiny comment but LGTM

// we need to bench here a bit, to see maybe it makes sense to use OrFilter
BoolQueryBuilder combined = new BoolQueryBuilder();
for (String aliasName : aliasNames) {
AliasMetaData alias = aliases.get(aliasName);

This comment has been minimized.

Copy link
@colings86

colings86 Oct 13, 2016

Member

Looks like there is an indentation issue here?

@colings86

This comment has been minimized.

Copy link
Member

commented Oct 13, 2016

Looks like org.elasticsearch.search.SearchServiceTests.testSearchWhileIndexDeleted failed in the PR build. Not sure if this failure is related to this change?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2016

@colings86 I added additional testing can you take another look?

@colings86
Copy link
Member

left a comment

Left 2 nit pick comments but LGTM

final Map<String, AliasFilter> queryBuilderMap = new HashMap<>();
for (String index : concreteIndices) {
clusterState.blocks().indexBlockedRaiseException(ClusterBlockLevel.READ, index);
AliasFilter queryBuilder = searchService.buildAliasFilter(clusterState, index, request.indices());

This comment has been minimized.

Copy link
@colings86

colings86 Oct 14, 2016

Member

nit: should this be called aliasFilter?

}

private Map<String, AliasFilter> buildPerIndexAliasFilter(SearchRequest request, ClusterState clusterState, String...concreteIndices) {
final Map<String, AliasFilter> queryBuilderMap = new HashMap<>();

This comment has been minimized.

Copy link
@colings86

colings86 Oct 14, 2016

Member

nit: should this be called aliasFilterMap?

@s1monw s1monw merged commit cff5993 into elastic:master Oct 14, 2016

1 check was pending

elasticsearch-ci Build started sha1 is merged.
Details

@s1monw s1monw deleted the s1monw:parse_alias_filter_early branch Oct 14, 2016

s1monw added a commit that referenced this pull request Oct 14, 2016

Parse alias filters on the coordinating node (#20916)
Today we don't parse alias filters on the coordinating node, we only forward
the alias patters to executing node and resolve it late. This has several problems
like requests that go through filtered aliases are never cached if they use date math,
since the parsing happens very late in the process even without rewriting. It also used
to be processed on every shard while we can only do it once per index on the coordinating node.
Another nice side-effect is that we are never prone to cluster-state updates that change an alias,
all nodes will execute the exact same alias filter since they are process based on the same
cluster state.

@javanna javanna referenced this pull request Nov 4, 2016

Closed

Refactor parsing of queries/filters, aggs, suggester APIs #10217

132 of 133 tasks complete
@javanna

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

@s1monw I think that we should add a bw comp layer on top of this change, which otherwise breaks backwards compatibility on the transport layer. Maybe our bw comp tests don't catch it because we have no REST tests that search, or validate query, or explain against a filtered alias?

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

@javanna you were wrong, this is fully bw compatible :)

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2016

@s1monw I think that we should add a bw comp layer on top of this change, which otherwise breaks backwards compatibility on the transport layer. Maybe our bw comp tests don't catch it because we have no REST tests that search, or validate query, or explain against a filtered alias?

the bwc layer is inside the new class... it's not very obvious from the high level but when you look closer you see that all kinds of BWC tests are present.

javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 22, 2016

Add indices and filter information to search shards api output
The search shards api returns info about which shards are going to be hit by executing a search with provided parameters: indices, routing, preference. Indices can also be aliases, which can also hold filters. The output includes an array of shards and a summary of all the nodes the shards are allocated on. This commit adds a new indices section to the search shards output that includes one entry per index, where each index can be associated with an optional filter in case the index was hit through a filtered alias.

This is relevant since we have moved parsing of alias filters to the coordinating node.

Relates to elastic#20916

javanna added a commit that referenced this pull request Nov 22, 2016

Add indices and filter information to search shards api output (#21738)
Add indices and filter information to search shards api output

The search shards api returns info about which shards are going to be hit by executing a search with provided parameters: indices, routing, preference. Indices can also be aliases, which can also hold filters. The output includes an array of shards and a summary of all the nodes the shards are allocated on. This commit adds a new indices section to the search shards output that includes one entry per index, where each index can be associated with an optional filter in case the index was hit through a filtered alias.

This is relevant since we have moved parsing of alias filters to the coordinating node.

Relates to #20916

javanna added a commit that referenced this pull request Nov 22, 2016

Add indices and filter information to search shards api output (#21738)
Add indices and filter information to search shards api output

The search shards api returns info about which shards are going to be hit by executing a search with provided parameters: indices, routing, preference. Indices can also be aliases, which can also hold filters. The output includes an array of shards and a summary of all the nodes the shards are allocated on. This commit adds a new indices section to the search shards output that includes one entry per index, where each index can be associated with an optional filter in case the index was hit through a filtered alias.

This is relevant since we have moved parsing of alias filters to the coordinating node.

Relates to #20916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.