Simplify counting in AbstractSearchAsyncAction #120593
Conversation
No need to do this so complicated, just count down one when we're actually done with a specific shard id.
original-brownbear
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
For others: This is a nice step on the way to #118490 (in addition to just being a considerable simplification) since it aligns the counting with the way it's done for batched execution in that PR. |
|
This also removes all uses of the group shards iterator interfaces and makes #116891 a trivial to understand cleanup. |
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Outdated
Show resolved
Hide resolved
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left a few comments. Shall we remove the two unused methods from GroupShardsIterator as well (totalSize and totalSizeWith1ForEmpty) ?
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Show resolved
Hide resolved
| // it's number of active shards but use 1 as the default if no replica of a shard is active at this point. | ||
| // on a per shards level we use shardIt.remaining() to increment the totalOps pointer but add 1 for the current shard result | ||
| // we process hence we add one for the non active partition here. | ||
| this.expectedTotalOps = shardsIts.totalSizeWith1ForEmpty(); |
There was a problem hiding this comment.
For posterity, this was introduced with faefc77 . The intent was to account for inactive shards (that are in the process of being allocated) as 1, so that they are expected to fail, otherwise their failure would make the search complete earlier than expected, missing results from other active shards.
Together with it SearchWhileCreatingIndexTests was added, which is now called SearchWhileCreatingIndexIT. The test went through a lot of changes and refactoring over time, and it was also quite problematic and flaky in the early days. Funnily enough, if I remove the counting of the empty group as 1 (and call totalSize instead), this specific test still succeeds. I may not have run it enough times to cause failures, or perhaps the issue that this was fixing no longer manifests. Either way, a lot of other tests fail due to too many ops executed compared to the expected ops, because the counting also needs to be adjusted accordingly (which is expected).
In principle, I agree that counting each shard as 1, regardless of how many copies it has whether that be inactive, primary only, one replica or multiple replicas is simpler.
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Show resolved
Hide resolved
| // but its range was available in the IndexMetadata, in that | ||
| // case the shardsIt.remaining() would be 0, expectedTotalOps | ||
| // accounts for unavailable shards too. | ||
| remainingOpsOnIterator = Math.max(shardsIt.remaining(), 1); |
There was a problem hiding this comment.
definitely good to get rid of this special case.
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Show resolved
Hide resolved
…into simplify-abstract-async-search
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a minor comment for readability. I would still remove the unused methods from GroupShardsIterator, leaving the removal of the entire class for a follow-up. If you prefer to leave the methods and do the removal later, that's also good, but I would not do it in this PR.
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Outdated
Show resolved
Hide resolved
|
Thanks Luca :) Removed the methods now and fixed the |
No need to do this so complicated, just count down one when we're actually done with a specific shard id.
3 participants
No need to do this so complicated, just count down one when we're actually done with a specific shard id.