[BUG] isDataNode method in DiscoveryNode returns true for any node which can contain data

[vinaykpud]

Describe the bug

Context

As part of discussion in #17422 issue we have now two new types of node roles : search and warm. Here is the PRs #17573 and #17620. We already have data role.

DiscoveryNodeRole has a boolean property canContainData which is true for search and warm nodes.

1. OpenSearch/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java

   Line 200 in d066743

   public static final DiscoveryNodeRole DATA_ROLE = new DiscoveryNodeRole("data", "d", true) {

2. OpenSearch/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java

   Line 316 in d066743

   public static final DiscoveryNodeRole SEARCH_ROLE = new DiscoveryNodeRole("search", "s", true) {

3. OpenSearch/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java

   Line 303 in d066743

   public static final DiscoveryNodeRole WARM_ROLE = new DiscoveryNodeRole("warm", "w", true) {

for all the other roles we have canContainData is false.

Issue

isDataNode() method in DiscoveryNode class decides if its data node based on the canContainData property of the DiscoveryNodeRole, as shown bellow:

OpenSearch/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java

Lines 113 to 115 in d066743

	public static boolean isDataNode(final Settings settings) {
	return getRolesFromSettings(settings).stream().anyMatch(DiscoveryNodeRole::canContainData);
	}

In DiscoveryNodes it uses the isDataNode() method to construct dataNodesBuilder map.

OpenSearch/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodes.java

Lines 812 to 814 in d066743

	if (nodeEntry.getValue().isDataNode()) {
	dataNodesBuilder.put(nodeEntry.getKey(), nodeEntry.getValue());
	}

So when we add search or warm node, they also counted as data nodes.

Solution

So we need to refactor to have two methods like this and use it accordingly depending on the usecase:

1. canContainData : Checks if the node with any role can contain data
2. isDataNode() : Checks if the node has data role

Before we can make above changes/refactor, I am trying to understand if we need to consider impact on any other components

@Bukhtawar @gbbafna @mch2 @andrross

Related component

Cluster Manager

[rajiv-kv]

[Triage - 1, 2, 3, 4]

Thanks @vinaykpud for filing the issue. Looks like the method isDataNode is used in multiple places and the intent might be to know if the node can contain data. Besides it is marked as PublicApi and would be breaking change as Plugin's might be also leveraging them.

Have you considered the option of introducing a new method for the role based check ?

[vinaykpud]

Yes @rajiv-kv , one option is

1. Rename existing isDataNode to canContainData : Checks if the node with any role can contain data
2. Add isDataNode() : Checks if the node has data role ie role based check

But this will be a breaking change.

Another option is:

1. Add a new method may be isDataRoleNode(), which checks if the node has data role

IMO, first option provides more clarity on the intention of method. Also other similar methods like isWarmNode() or isSearchNode() does role based check.

[mch2]

@vinaykpud, where exactly do we need to differentiate here between "able to contain data" and "is a data node role" that is breaking search/warm functionality? Seems in those locations we should have logic specifically applying to search/warm roles instead?