Remove GroupShardsIterator and replace it with plain List #116891
Conversation
There is no point in having `GroupShardsIterator`, it's mostly an unnecessary layer of indirection as it has no state and a single field only. It's only value could be seen in it hiding the ability to mutate the list it wraps, but that hardly justifies the overhead on the search path and extra code complexity. Moreover, the list it references is not copied/immutable in any way, so the value of hiding is limited also. Unwrapping all its use to list and moving its methods (mostly single use or test-only) elsewhere/inline saves a lot of code, makes the logic easier to follow by removing a step, and sets up further optimizations (we currently copy lists unnecessarily to map one `GroupShardsIterator` to another in a few spots, we could just mutate lists in place here mostly).
original-brownbear
added
>non-issue
:Search Foundations/Search
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
added
Team:Search Foundations
|
|
||
| import static org.elasticsearch.cluster.routing.ShardRoutingState.STARTED; | ||
|
|
||
| public class GroupShardsIteratorTests extends ESTestCase { |
There was a problem hiding this comment.
Outside of the two test utilities in this class, I don't believe there is any value in the tests for the methods we had. totalSize was test-only, the count 1 for empty method only had a single well-covered callsite in production and testing iteration and sorting is unnecessary for lists and covered by existing tests anyway.
|
@elasticmachine update branch |
|
There are no new commits on the base branch. |
|
@elasticmachine update branch |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a couple of minors, LGTM otherwise
server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/SearchProgressListener.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
Show resolved
Hide resolved
| } | ||
| return GroupShardsIterator.sortAndCreate(new ArrayList<>(set)); | ||
| var res = new ArrayList<>(set); | ||
| CollectionUtil.timSort(res); |
There was a problem hiding this comment.
@javanna see my other comment this one in particular seems unnecesssary. We already sort the iterators in the search action, we shouldn't need to sort them here as well?
original-brownbear
added
auto-backport
|
Thanks Luca! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
|
Would you mind pinging me once you backported to 8.x? I have one that depends on it. |
|
Heads up: I removed the 9.0 label, I don't think this should be backported to 9.0. |
…122253) There is no point in having `GroupShardsIterator`, it's mostly an unnecessary layer of indirection as it has no state and a single field only. It's only value could be seen in it hiding the ability to mutate the list it wraps, but that hardly justifies the overhead on the search path and extra code complexity. Moreover, the list it references is not copied/immutable in any way, so the value of hiding is limited also.
4 participants
There is no point in having
GroupShardsIterator, it's mostly an unnecessary layer of indirection as it has no state and a single field only. It's only value could be seen in it hiding the ability to mutate the list it wraps, but that hardly justifies the overhead on the search path and extra code complexity. Moreover, the list it references is not copied/immutable in any way, so the value of hiding is limited also.