Iterate directly over contents of RoutingNode
#137694
Conversation
Today `RoutingNode#initializing`, `RoutingNode#started` and `RoutingNode#relocating` all work by copying the result into a freshly allocated array. These collections are immutable and all the (production) callers immediately iterate over the result so the copy is unnecessary. Moreover the `started` collection in particular might be huge. This commit adjusts these methods to return an iterator over the underlying collection instead, asserting that the resulting iterators are never mutated. Closes ES-13364
DaveCTurner
added
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Looks good though I'd find Iterable slightly better?
|
|
||
| public ShardRouting[] initializing() { | ||
| return initializingShards.toArray(EMPTY_SHARD_ROUTING_ARRAY); | ||
| public Iterator<ShardRouting> initializing() { |
There was a problem hiding this comment.
I'd find it slightly more intuitive to return an Iterable here, to preserve the native for loop construct rather than the old while iterator style. Is there a reason you went for the iterator?
There was a problem hiding this comment.
Iterable implies you can iterate it more than once but we don't actually do so anywhere. Also a bit more of a pain to assert nobody tries to remove anything from the resulting iterator. Not a watertight argument but we do have more utilities for working with an Iterator too.
There was a problem hiding this comment.
a bit more of a pain to assert nobody tries to remove anything from the resulting iterator
Not sure I follow, would it not simply (or just 🧌) be implemented by:
() -> Iterators.assertReadOnly(initializingShards.iterator())
i.e., the implementation is straightforward (and we avoid the 3 lines of code per for loop).
There was a problem hiding this comment.
Also skipping this lambda construction when assertions disabled but yeah only a bit more of a pain.
| assert initializingShardsIterator.hasNext(); | ||
| final var initializingShard = initializingShardsIterator.next(); | ||
| assert initializingShardsIterator.hasNext() == false | ||
| : "expect exactly one relocating shard, but got: " + Iterators.toList(initializingShardsIterator); |
There was a problem hiding this comment.
Nit:
| : "expect exactly one relocating shard, but got: " + Iterators.toList(initializingShardsIterator); | |
| : "expect exactly one relocating shard, but got: [" + initializingShard + "] and " + Iterators.toList(initializingShardsIterator); |
4 participants
Today
RoutingNode#initializing,RoutingNode#startedandRoutingNode#relocatingall work by copying the result into a freshlyallocated array. These collections are immutable and all the
(production) callers immediately iterate over the result so the copy is
unnecessary. Moreover the
startedcollection in particular might behuge.
This commit adjusts these methods to return an iterator over the
underlying collection instead, asserting that the resulting iterators
are never mutated.
Closes ES-13364