DiscoveryNodes resolver resolves to all nodes if an empty list is given

[hendrikmuhs]

This logic needs clarification and seems inconsistent:

DiscoveryNodes.resolver resolves an empty list of node id's to all nodes:

https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java#L329

This is in contrast to consumers of this API which explicitly check if the resolved nodes have length == 0, e.g.

https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/support/tasks/TransportTasksAction.java#L241

Due to the implicit to all resolving this can never happen.

It seems to me that:

• only null should trigger a resolve to all nodes but
• an empty array should resolve to an empty array of nodes

This is potentially a tricky change. If you disagree, meaning if the current behavior is desired it would be good to clarify and document this.

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/Cluster Coordination)

[DaveCTurner]

DiscoveryNodes#resolve implements the behaviour of the various APIs that let you apply node filters (e.g. nodes info, task management). IMO I don’t think we should have different (meaningful) behaviours for null vs an empty list since you can’t really distinguish these cases at the API level, although I would be happy if we simply rejected null with an NPE here. The reference manual has fairly comprehensive documentation of the behaviour, including this sentence:

if no filters are given, the default is to select all nodes

The in-code docs for this method are admittedly much less comprehensive.

This method can return an empty set of nodes in a number of ways. You could for instance pass a filter that doesn't match any nodes (e.g. not-a-node-name, or _all,master:false in a one-node cluster) or pass an inconsistent sequence of filters (e.g. data:true,data:false), or even a single negative filter (e.g. ingest:false).

So I think the current behaviour is desired and documented, and callers are correct to expect it to return an empty set of nodes sometimes. I see room for improvement in the Javadoc for this method and IMO rejecting null would be a good move.

[DaveCTurner]

We discussed this in the team meeting yesterday and decided that I'll open a PR to improve the docs on this method in due course.

[hendrikmuhs]

@DaveCTurner Thanks for clarification. In order to close this issue with #53277 I created a follow up issue: #53281, because your doc fix does not solve the root cause.