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

Cross Cluster Search: propagate original indices per cluster #24328

Merged
merged 13 commits into from Apr 26, 2017

Conversation

Projects
None yet
2 participants
@javanna
Member

javanna commented Apr 26, 2017

In case of a Cross Cluster Search, the coordinating node should split the original indices per cluster, and send over to each cluster only its own set of original indices, rather than set taken from the original search request which contains all the indices.

In fact, each remote cluster should not be aware of the indices belonging to other remote clusters.

@s1monw

i left some minors looks good man!

core/src/main/java/org/elasticsearch/search/SearchService.java Outdated
throws IOException {
IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex());
IndexShard indexShard = indexService.getShard(request.shardId().getId());
SearchShardTarget shardTarget = new SearchShardTarget(clusterService.localNode().getId(), indexShard.shardId());
SearchShardTarget shardTarget = new SearchShardTarget(clusterService.localNode().getId(), indexShard.shardId(), originalIndices);

This comment has been minimized.

@s1monw

s1monw Apr 26, 2017

Contributor

I don't think we need this here at all lets pass always null here to the SearchShardTarget

This comment has been minimized.

@javanna

javanna Apr 26, 2017

Member

thanks for bringing this up, I was not too sure about this.

core/src/main/java/org/elasticsearch/cluster/routing/GroupShardsIterator.java Outdated
@@ -30,14 +30,14 @@
* ShardsIterators are always returned in ascending order independently of their order at construction
* time. The incoming iterators are sorted to ensure consistent iteration behavior across Nodes / JVMs.
*/
public final class GroupShardsIterator implements Iterable<ShardIterator> {
public final class GroupShardsIterator<S extends ShardIterator> implements Iterable<S> {

This comment has been minimized.

@s1monw

s1monw Apr 26, 2017

Contributor

s/S/ShardIt

@@ -112,7 +112,7 @@ private void innerRun() throws IOException {
final IntArrayList[] docIdsToLoad = searchPhaseController.fillDocIdsToLoad(numShards, reducedQueryPhase.scoreDocs);
if (reducedQueryPhase.scoreDocs.length == 0) { // no docs to fetch -- sidestep everything and return
phaseResults.stream()
.map(e -> e.queryResult())
.map(SearchPhaseResult::queryResult)

This comment has been minimized.

@s1monw

s1monw Apr 26, 2017

Contributor

👍

core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java Outdated
shards.add(shardIterator);
}
for (ShardIterator shardIterator : localShardsIterator) {
shards.add(shardIterator);
List<ShardRouting> shardRoutings = new ArrayList<>();
for (ShardRouting shardRouting : shardIterator.asUnordered()) {

This comment has been minimized.

@s1monw

s1monw Apr 26, 2017

Contributor

I hate this copying here :) can we maybe do this:

  • make ShardsIterator extends Iterable<ShardRouting>
  • remove ShardsIterator#asUnordered()
  • add List<ShardRouting> ShardsIterator#getShardRoutings()

that way we can just reuse the list?

This comment has been minimized.

@javanna

javanna Apr 26, 2017

Member

loving this plan. I don't like this copying either.

@javanna

This comment has been minimized.

Member

javanna commented Apr 26, 2017

@s1monw I addressed your comments and added some tests. Can you have another look please?

@s1monw

s1monw approved these changes Apr 26, 2017

left one suggestion LGTM otherwise. no need for another review. Thanks for the additional tests

@@ -74,7 +75,12 @@ public int sizeActive() {
}
@Override
public Iterable<ShardRouting> asUnordered() {
public List<ShardRouting> getShardRoutings() {
return shards;

This comment has been minimized.

@s1monw

s1monw Apr 26, 2017

Contributor

Maybe wrap it in an unmodifiable list in the ctor? I think we should not allow mutation here?

This comment has been minimized.

@javanna

javanna Apr 26, 2017

Member

++ will do

@javanna javanna merged commit 149629f into elastic:master Apr 26, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

javanna added a commit that referenced this pull request Apr 26, 2017

Cross Cluster Search: propagate original indices per cluster (#24328)
In case of a Cross Cluster Search, the coordinating node should split the original indices per cluster, and send over to each cluster only its own set of original indices, rather than the set taken from the original search request which contains all the indices.

In fact, each remote cluster should not be aware of the indices belonging to other remote clusters.

javanna added a commit that referenced this pull request Apr 26, 2017

Cross Cluster Search: propagate original indices per cluster (#24328)
In case of a Cross Cluster Search, the coordinating node should split the original indices per cluster, and send over to each cluster only its own set of original indices, rather than the set taken from the original search request which contains all the indices.

In fact, each remote cluster should not be aware of the indices belonging to other remote clusters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment