Fix search shards count method when targeting concrete and aliased indices

[Paikan]

This is related to the same example as issue #2682.

This bug can lead to a forced query_and_fetch search type which might not be the expected behavior.

I propose to fix it this way because it is a quick 2 lines patch. But maybe we should refactor a bit and add a shared method for searchShards and searchShardsCount to avoid divergent behavior in the future.

Reading the code it seems like searchShardsCount is mainly implemented to test if we are hitting 1 shard or more than 1 shards so maybe it could be renamed and optimized for that specific use case.

Feel free to fix the problem otherwise or to tell me what do you prefer and I will be happy to change this pull request.

[javanna]

Thanks for pointing this out! Let me just check if I got it right. The searchShards method has been fixed (I see that #2682 is closed and the related #2683 has been merged) but the same change has not been applied to the searchShardsCount method, right? And that would lead to erroneously forcing query_and_fetch if the searchShardsCount returns 1?

I guess it would be nice to share the same code between the two methods then and maybe write a test that reproduces the bug that we are trying to address. What do you think?

[Paikan]

@javanna yeah you got it perfectly right!

Yeah I agree I can work on it and update my pull request hopefully before the end of the week if you want. If you want it done before it is ok if you prefer to implement it yourself

[javanna]

No rush, you can go ahead and update your pull request when you have time.

[Paikan]

@javanna great thanks I will

[kimchy]

hey, I suggest we pull this fix first, just so we have a fix in master and 0.90, and think about optimization / refactoring in a separate issue?

One note regarding potentially sharing code, its important that we don't cause the round robin iterators be invoked when calling searchShardsCount, cause if they will, it will mean that with the common case, of 1 replica, the same copy of the data will be hit each time (a single search request will increment the round robin index by 1 for the count, and then the next one will increment it again for the search).

[Paikan]

@kimchy sure we can push this fix and work on the refactoring in a separate issue.

I was about to update the pull request with the refactoring tomorrow (I am still missing the unit tests for it). Basically what I have done is create a new private method Set computeTargetedShards(ClusterState clusterState, String[] indices, String[] concreteIndices, @nullable Map<String, Set> routing) with the shared logic in it. It is called by searchShardsCount and searchShards.

[Paikan]

@kimchy sorry a part is missing there the computeTargetedShards would return a set of IndexShardRoutingTable

[kimchy]

@Paikan I am ok with the current pull request, then @javanna, lets pull this in once you double check it. Then, lets open a new pull request with the suggested refactoring, and discuss it there.

[Paikan]

@kimchy ok I am going to open another issue with my proposed pull request tomorrow and we will be able to discuss more there about the suggested refactoring and maybe ways to optimize / improve it

[javanna]

Merged into 8e137b1 and backported to 0.90. Thanks @Paikan !!
I've also added a test for it...have a look at 7f7f79d if you're interested ;)

[Paikan]

@javanna thanks for pushing this and for adding the missing unit test it was quite interesting indeed.

@kimchy I have opened and updated PR #3530 to discuss about refactoring.