Small for-loop cleanup #120131
Small for-loop cleanup #120131
Conversation
elasticsearchmachine
added
v9.0.0
needs:triage
DiannaHohensee
force-pushed
the
2025/01/14/ES-10341-cleanup
branch
from
8b13d12 to
160d763
Compare
DiannaHohensee
added
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| if (node != null) { | ||
| filteredNodeAllocationStatsAndWeights.put(node, nodeStatsAndWeight.getValue()); | ||
| } |
There was a problem hiding this comment.
nodesAllocationStatsAndWeights uses RoutingNodes, but RoutingNode can have at most one DiscoveryNode or null (I'm looking at Nullable node property). Potentially allocation.nodes().get(nodeStatsAndWeight.getKey()); might return null when RoutingNode does not have DiscoveryNode specified, right?
There was a problem hiding this comment.
Thanks for looking into it. Following the allocation.routingNodes() and allocation.nodes() methods down, it looks like the routing nodes are instantiated here. It looks reasonable that the routing nodes has all of the nodes, from this code, but there are no interface guarantees that I see to feel good about that conclusion..
Looks like the Nullable was added in this commit, probably as a performance edge case.
Eh. I feel like it should have all the nodes, but also there are no guarantees around that, so this change probably isn't safe as the RoutingNodes code is today.
There was a problem hiding this comment.
Do you want to pursue this PR or close it?
3 participants
The same RoutingAllocation instance is used to supply the nodes to both maps, so it should be safe to assume that we'll never find a node in one map and not the other. This is purely a transformational for-loop, it is not filtering anything out.