You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/12/21 00:48:08 UTC

[GitHub] [solr] murblanc commented on a change in pull request #403: SOLR-15694 Concept of node roles and non-data nodes

murblanc commented on a change in pull request #403:
URL: https://github.com/apache/solr/pull/403#discussion_r772475914



##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";

Review comment:
       The default should be the defaults of each role rather than a string with a list of roles and values. this string should be removed.
   
   The issue with such a string is that if a single role is defined on a node, the change of roles of that node might impact other roles as they'll change from having the value defined in this string to the default for the role.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -243,12 +247,24 @@ private static boolean existCoreName(String coreName, Slice slice) {
       }
     } else {
       nodeList = new ArrayList<>(liveNodes);
+      filterNonDataNodes(zk, nodeList);
       Collections.shuffle(nodeList, random);
     }
 
     return nodeList;
   }
 
+  public static void filterNonDataNodes(DistribStateManager zk, List<String> liveNodes) {
+    try {
+     List<String> noData =  ClusterAPI.getNodesByRole(NodeRoles.Role.DATA, NodeRoles.OFF, zk);
+     if(!noData.isEmpty()){

Review comment:
       formatting: add space after `if`.
   But that `if` should be removed altogether, `removeAll()` will remove nothing if list is empty.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+      }
+    }
+    for(Role r: Role.values()) {
+      if (!roles.containsKey(r)) {
+        roles.put(r, r.defaultIfAbsent());
+      }
+    }
+    nodeRoles = Collections.unmodifiableMap(roles);
+  }
+
+  public Map<Role, String> getRoles() {
+    return nodeRoles;
+  }
+
+  public String getRoleMode(Role role) {
+    return nodeRoles.get(role);
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+  }
+
+  public boolean isOverseerAllowed() {
+    String roleMode = nodeRoles.get(Role.OVERSEER);
+    return ALLOWED.equals(roleMode) || PREFERRED.equals(roleMode);
+  }
+
+  public enum Role {
+    DATA("data"),
+    OVERSEER("overseer") {
+      @Override
+      public Set<String> supportedModes() {
+        return OVERSEER_MODES;
+      }
+      @Override
+      public String defaultIfAbsent() {
+        return DISALLOWED;
+      }
+    };
+
+    public final String roleName;
+
+    Role(String name) {
+      this.roleName = name;
+    }
+
+    public static Role getRole(String value) {
+      for (Role role: Role.values()) {
+        if (value.equals(role.roleName)) return role;
+      }
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value);
+    }
+
+    public Set<String> supportedModes() {
+      return ON_OFF;
+    }
+
+    /**
+     * Default mode for a role in nodes where this role is not specified.
+     */
+    public String defaultIfAbsent() {
+      return OFF;
+    }
+
+    @Override
+    public String toString() {
+      return super.toString().toLowerCase(Locale.ROOT);

Review comment:
       Any reason for this specific override, related to `MapWriter` maybe? If so, please comment.
   But if we rely on `roleName` then just return it.
   
   If no specific reason, I suggest we let the default `toString()` implementation return the enum name, so just remove this override.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;

Review comment:
       Here if no roles are passed, I'd use the `defaultIfAbsent` of all the roles (i.e. if `null`, set to empty string, the logic below will take care of the rest).

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;
+    try {
+      List<String> children = zk.listData(path);
+      if (children != null && !children.isEmpty()) {
+        result = new HashMap<>();
+      } else {
+        return depth >= 1 ? Collections.emptyList() : null;
+      }
+      for (String child : children) {
+        Object c = readRecursive(path + "/" + child, zk, depth - 1);
+        result.put(child, c);
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+    }
+    if (depth == 1) {
+      return result.keySet();
+    } else {
+      return result;
+    }
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRole(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    rsp.add("node-roles", Map.of(role,
+            readRecursive(ZkStateReader.NODE_ROLES + "/"+ role,
+                    collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/node/{node}",
+          permission = COLL_READ_PERM)
+  @SuppressWarnings("unchecked")
+  public void rolesForNode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String node = req.getPathTemplateValues().get("node");
+    Map<String, String> ret = new HashMap<String, String>();
+    Map<String, Map<String, Set<String>>> roles = (Map<String, Map<String, Set<String>>>) readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3);
+    for (String role: roles.keySet()) {
+      for (String mode : roles.get(role).keySet()) {
+        if (roles.get(role).get(mode) instanceof List && ((List) roles.get(role).get(mode)).size() == 0) {
+          continue;
+        }
+        Set<String> nodes = roles.get(role).get(mode);
+        if (nodes.contains(node)) ret.put(role, mode);
+      }
+    }
+    for (String role: ret.keySet()) {
+      rsp.add(role, ret.get(role));
+    }
+  }
+
   @EndPoint(method = GET,
-      path = "/cluster/aliases",
-      permission = COLL_READ_PERM)
+          path = "/cluster/node-roles/supported",
+          permission = COLL_READ_PERM)
+  public void supportedRoles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+     Map<String, Object> roleModesSupportedMap = new HashMap<>();
+    for (NodeRoles.Role role: NodeRoles.Role.values()) {
+      roleModesSupportedMap.put(role.toString(), Map.of("modes", role.supportedModes(), "defaultIfAbsent", role.defaultIfAbsent()));
+    }
+    rsp.add("supported-roles", roleModesSupportedMap);
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}/{role-val}",

Review comment:
       Do roles have "modes" or "values"? Need consistency. Here it's value, in `NodeRoles` it's modes.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));

Review comment:
       Need to add in the message the role name for which an unknown mode is configured.

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -944,6 +946,14 @@ public void load() {
       });
 
       clusterSingletons.setReady();
+      if (NodeRoles.PREFERRED.equals(nodeRoles.getRoleMode(NodeRoles.Role.OVERSEER))) {
+        try {
+          log.info("This node is started as a preferred overseer");
+          zkSys.getZkController().setPreferredOverseer();
+        } catch (KeeperException | InterruptedException e) {
+          throw new SolrException(ErrorCode.SERVER_ERROR, e);
+        }
+      }
       if (!distributedCollectionCommandRunner.isPresent()) {

Review comment:
       The added bloc above must be moved inside this if otherwise we will be sending a message over the overseer queue when no overseer is running to consume it.

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -913,7 +924,9 @@ private void init() {
         ElectionContext context = new OverseerElectionContext(zkClient,
             overseer, getNodeName());
         overseerElector.setup(context);
-        overseerElector.joinElection(context, false);
+        if(cc.nodeRoles.isOverseerAllowed()) {

Review comment:
       formatting: please add space after `if`.

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerNodePrioritizer.java
##########
@@ -59,11 +63,18 @@ public OverseerNodePrioritizer(ZkStateReader zkStateReader, Overseer overseer, S
 
   public synchronized void prioritizeOverseerNodes(String overseerId) throws Exception {
     SolrZkClient zk = zkStateReader.getZkClient();
-    if(!zk.exists(ZkStateReader.ROLES,true))return;
-    Map<?,?> m = (Map<?,?>) Utils.fromJSON(zk.getData(ZkStateReader.ROLES, null, new Stat(), true));
+    List<String> overseerDesignates = new ArrayList<>();
+    if (zk.exists(ZkStateReader.ROLES,true)) {
+      Map<?,?> m = (Map<?,?>) Utils.fromJSON(zk.getData(ZkStateReader.ROLES, null, new Stat(), true));
+      @SuppressWarnings("unchecked")
+      List<String> l = (List<String>) m.get("overseer");
+      if (l != null) {
+        overseerDesignates.addAll(l);
+      }
+    }
 
-    List<?> overseerDesignates = (List<?>) m.get("overseer");
-    if(overseerDesignates==null || overseerDesignates.isEmpty()) return;
+    overseerDesignates.addAll(ClusterAPI.getNodesByRole(NodeRoles.Role.OVERSEER, NodeRoles.PREFERRED, new ZkDistribStateManager(zkStateReader.getZkClient())));

Review comment:
       I'd log a warning if Overseer role configured in both `/roles.json` and in (new) node roles.

##########
File path: solr/solr-ref-guide/src/node-roles.adoc
##########
@@ -0,0 +1,198 @@
+= Node Roles
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+A node in Solr is usually capable of performing various types of operations, e.g. hosting replicas, performing indexing and querying on them, collection management tasks etc. However, if someone wants to setup a cluster where these functionalities are isolated to certain dedicated nodes, then this can be achieved leveraging the concept of node roles.
+
+== Definitions
+
+=== Node role
+
+A role is a designation of a node that indicates that the node may perform a certain functionality that is governed by the role.
+
+=== Mode
+Every role has a list of modes under which a node can be. It can be simple (e.g. `["on", "off"]`) or more granular (e.g. `["allowed", "preferred", "disallowed"]`).
+
+== Roles
+
+In order to specify role(s) for a node, one needs to start a Solr node with the following parameter.
+
+.Startup Parameter
+[cols="1,2,1,1"] 
+|===
+|Parameter |Value |Required? | Default
+
+|solr.node.roles
+|Comma separated list of roles (in the format: `<role>:<mode>`) for this node.
+Examples: `-Dsolr.node.roles=data:on,overseer:allowed` or `-Dsolr.node.roles=overseer:preferred`
+|No
+|`data:on,overseer:allowed`
+|===
+
+[TIP]
+====
+If a node has been started with no `solr.node.roles` parameter, it will be assumed to have the data role turned on and overseer role allowed on it. If you've never used roles before, you likely won't need to change anything in your startup parameters to accomodate the functionality associated with these roles.

Review comment:
       typo: accommodate (two m)

##########
File path: solr/solr-ref-guide/src/node-roles.adoc
##########
@@ -0,0 +1,198 @@
+= Node Roles
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+A node in Solr is usually capable of performing various types of operations, e.g. hosting replicas, performing indexing and querying on them, collection management tasks etc. However, if someone wants to setup a cluster where these functionalities are isolated to certain dedicated nodes, then this can be achieved leveraging the concept of node roles.
+
+== Definitions
+
+=== Node role
+
+A role is a designation of a node that indicates that the node may perform a certain functionality that is governed by the role.
+
+=== Mode
+Every role has a list of modes under which a node can be. It can be simple (e.g. `["on", "off"]`) or more granular (e.g. `["allowed", "preferred", "disallowed"]`).
+
+== Roles
+
+In order to specify role(s) for a node, one needs to start a Solr node with the following parameter.
+
+.Startup Parameter
+[cols="1,2,1,1"] 
+|===
+|Parameter |Value |Required? | Default
+
+|solr.node.roles
+|Comma separated list of roles (in the format: `<role>:<mode>`) for this node.
+Examples: `-Dsolr.node.roles=data:on,overseer:allowed` or `-Dsolr.node.roles=overseer:preferred`
+|No
+|`data:on,overseer:allowed`
+|===
+
+[TIP]
+====
+If a node has been started with no `solr.node.roles` parameter, it will be assumed to have the data role turned on and overseer role allowed on it. If you've never used roles before, you likely won't need to change anything in your startup parameters to accomodate the functionality associated with these roles.
+====
+
+.Supported roles
+[cols="1,1"] 
+|===
+|Role |Modes
+
+|`data`
+|on, off
+
+|`overseer`
+|allowed, preferred, disallowed
+|===
+
+=== `data` role
+A node with this role (in mode "on") can host shards and replicas for collections.
+
+=== `overseer` role
+A node with this role can perform duties of an overseer node (unless mode is `disallowed`). When one or more nodes have the overseer role in `preferred` mode, the overseer leader will be elected from one of these nodes. In case no node is designated as a preferred overseer or no such node is live, the overseer leader will be elected from one of the nodes that have the overseer role in `allowed` mode. If all nodes that are designated with overseer role (allowed or preferred) are down, the cluster will be left without an overseer.
+
+== Example usage
+
+Sometimes, when the nodes in a cluster are under heavy querying or indexing load, the overseer leader node might be unable to perform collection management duties efficiently. It might be reasonable to have dedicated nodes to act as the overseer. Such an effect can be achieved as follows:
+
+* Most nodes (data nodes) in the cluster start with `-Dsolr.node.roles=data:on,overseer:allowed` (or with no parameter, since the default value for `solr.node.roles` is the same).

Review comment:
       If you accept that the default should be per role (and I hope you do, it doesn't change cluster behavior with this PR but simplifies life for later), this sentence has to be rephrased (see comment line 41 of `NodeRoles`)

##########
File path: solr/solr-ref-guide/src/node-roles.adoc
##########
@@ -0,0 +1,198 @@
+= Node Roles
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+A node in Solr is usually capable of performing various types of operations, e.g. hosting replicas, performing indexing and querying on them, collection management tasks etc. However, if someone wants to setup a cluster where these functionalities are isolated to certain dedicated nodes, then this can be achieved leveraging the concept of node roles.
+
+== Definitions
+
+=== Node role
+
+A role is a designation of a node that indicates that the node may perform a certain functionality that is governed by the role.
+
+=== Mode
+Every role has a list of modes under which a node can be. It can be simple (e.g. `["on", "off"]`) or more granular (e.g. `["allowed", "preferred", "disallowed"]`).
+
+== Roles
+
+In order to specify role(s) for a node, one needs to start a Solr node with the following parameter.
+
+.Startup Parameter
+[cols="1,2,1,1"] 
+|===
+|Parameter |Value |Required? | Default
+
+|solr.node.roles
+|Comma separated list of roles (in the format: `<role>:<mode>`) for this node.
+Examples: `-Dsolr.node.roles=data:on,overseer:allowed` or `-Dsolr.node.roles=overseer:preferred`
+|No
+|`data:on,overseer:allowed`
+|===
+
+[TIP]
+====
+If a node has been started with no `solr.node.roles` parameter, it will be assumed to have the data role turned on and overseer role allowed on it. If you've never used roles before, you likely won't need to change anything in your startup parameters to accomodate the functionality associated with these roles.
+====
+
+.Supported roles

Review comment:
       Table should indicate default value for each role.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {

Review comment:
       Why is `NodeRoles` a `MapWriter`?

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+      }
+    }
+    for(Role r: Role.values()) {
+      if (!roles.containsKey(r)) {
+        roles.put(r, r.defaultIfAbsent());
+      }
+    }
+    nodeRoles = Collections.unmodifiableMap(roles);
+  }
+
+  public Map<Role, String> getRoles() {
+    return nodeRoles;
+  }
+
+  public String getRoleMode(Role role) {
+    return nodeRoles.get(role);
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+  }
+
+  public boolean isOverseerAllowed() {
+    String roleMode = nodeRoles.get(Role.OVERSEER);
+    return ALLOWED.equals(roleMode) || PREFERRED.equals(roleMode);
+  }
+
+  public enum Role {
+    DATA("data"),
+    OVERSEER("overseer") {
+      @Override
+      public Set<String> supportedModes() {
+        return OVERSEER_MODES;
+      }
+      @Override
+      public String defaultIfAbsent() {
+        return DISALLOWED;
+      }
+    };
+
+    public final String roleName;
+
+    Role(String name) {
+      this.roleName = name;
+    }
+
+    public static Role getRole(String value) {
+      for (Role role: Role.values()) {
+        if (value.equals(role.roleName)) return role;
+      }
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value);
+    }
+
+    public Set<String> supportedModes() {
+      return ON_OFF;
+    }
+
+    /**
+     * Default mode for a role in nodes where this role is not specified.
+     */
+    public String defaultIfAbsent() {

Review comment:
       I would make `abstract` this method as well as `supportedModes()` above, so each `Role` has to define them in the enum. Less error prone and easier to understand the logic of each role.
   
   I'd also rename this one into `defaultMode()`. "If absent" is redundant since we use the default only if the role is absent (i.e. not specified).

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -243,12 +247,24 @@ private static boolean existCoreName(String coreName, Slice slice) {
       }
     } else {
       nodeList = new ArrayList<>(liveNodes);
+      filterNonDataNodes(zk, nodeList);
       Collections.shuffle(nodeList, random);
     }
 
     return nodeList;
   }
 
+  public static void filterNonDataNodes(DistribStateManager zk, List<String> liveNodes) {

Review comment:
       This method should be moved to class `NodeRoles` (or another one if we want to separate single node role checks and multi node role checks) and be implemented in a more generic way (passing the role and passing the role value to filter on).

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+      }
+    }
+    for(Role r: Role.values()) {
+      if (!roles.containsKey(r)) {
+        roles.put(r, r.defaultIfAbsent());
+      }
+    }
+    nodeRoles = Collections.unmodifiableMap(roles);
+  }
+
+  public Map<Role, String> getRoles() {
+    return nodeRoles;
+  }
+
+  public String getRoleMode(Role role) {
+    return nodeRoles.get(role);
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+  }
+
+  public boolean isOverseerAllowed() {
+    String roleMode = nodeRoles.get(Role.OVERSEER);
+    return ALLOWED.equals(roleMode) || PREFERRED.equals(roleMode);
+  }
+
+  public enum Role {
+    DATA("data"),
+    OVERSEER("overseer") {
+      @Override
+      public Set<String> supportedModes() {
+        return OVERSEER_MODES;

Review comment:
       There should be no constant `OVERSEER_MODES` and the only way to get to these values should be by doing `Role.OVERSEER.supportedModes()`.
   
   This line should therefore be: `return Set.of(ALLOWED, PREFERRED, DISALLOWED);`

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+      }
+    }
+    for(Role r: Role.values()) {
+      if (!roles.containsKey(r)) {
+        roles.put(r, r.defaultIfAbsent());
+      }
+    }
+    nodeRoles = Collections.unmodifiableMap(roles);
+  }
+
+  public Map<Role, String> getRoles() {
+    return nodeRoles;
+  }
+
+  public String getRoleMode(Role role) {
+    return nodeRoles.get(role);
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+  }
+
+  public boolean isOverseerAllowed() {
+    String roleMode = nodeRoles.get(Role.OVERSEER);
+    return ALLOWED.equals(roleMode) || PREFERRED.equals(roleMode);
+  }
+
+  public enum Role {
+    DATA("data"),
+    OVERSEER("overseer") {
+      @Override
+      public Set<String> supportedModes() {
+        return OVERSEER_MODES;
+      }
+      @Override
+      public String defaultIfAbsent() {
+        return DISALLOWED;
+      }
+    };
+
+    public final String roleName;
+
+    Role(String name) {
+      this.roleName = name;
+    }
+
+    public static Role getRole(String value) {
+      for (Role role: Role.values()) {

Review comment:
       I'd simplify and make the enum name be used as the role name, by using Role.valueOf(value.toUpperCase()).

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;

Review comment:
       initialization not needed here.

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;
+    try {
+      List<String> children = zk.listData(path);
+      if (children != null && !children.isEmpty()) {
+        result = new HashMap<>();
+      } else {
+        return depth >= 1 ? Collections.emptyList() : null;
+      }
+      for (String child : children) {
+        Object c = readRecursive(path + "/" + child, zk, depth - 1);
+        result.put(child, c);
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+    }
+    if (depth == 1) {
+      return result.keySet();
+    } else {
+      return result;
+    }
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRole(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    rsp.add("node-roles", Map.of(role,
+            readRecursive(ZkStateReader.NODE_ROLES + "/"+ role,
+                    collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/node/{node}",
+          permission = COLL_READ_PERM)
+  @SuppressWarnings("unchecked")
+  public void rolesForNode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String node = req.getPathTemplateValues().get("node");
+    Map<String, String> ret = new HashMap<String, String>();
+    Map<String, Map<String, Set<String>>> roles = (Map<String, Map<String, Set<String>>>) readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3);
+    for (String role: roles.keySet()) {
+      for (String mode : roles.get(role).keySet()) {
+        if (roles.get(role).get(mode) instanceof List && ((List) roles.get(role).get(mode)).size() == 0) {

Review comment:
       How can this ever be a `List`?

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();

Review comment:
       unused variable

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;
+    try {
+      List<String> children = zk.listData(path);
+      if (children != null && !children.isEmpty()) {
+        result = new HashMap<>();
+      } else {
+        return depth >= 1 ? Collections.emptyList() : null;
+      }
+      for (String child : children) {
+        Object c = readRecursive(path + "/" + child, zk, depth - 1);
+        result.put(child, c);
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+    }
+    if (depth == 1) {
+      return result.keySet();
+    } else {
+      return result;
+    }
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRole(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    rsp.add("node-roles", Map.of(role,
+            readRecursive(ZkStateReader.NODE_ROLES + "/"+ role,
+                    collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/node/{node}",
+          permission = COLL_READ_PERM)
+  @SuppressWarnings("unchecked")
+  public void rolesForNode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String node = req.getPathTemplateValues().get("node");
+    Map<String, String> ret = new HashMap<String, String>();
+    Map<String, Map<String, Set<String>>> roles = (Map<String, Map<String, Set<String>>>) readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3);
+    for (String role: roles.keySet()) {

Review comment:
       Seems a bit wasteful to read all the info from ZK to then throw away most of it and only retain the data pertaining to a single node. We could add some basic filtering logic in `readRecursive` to limit to a given node.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";

Review comment:
       Code would be more structured if node roles are defined in a subclass of `NodeRoles`.
   So a role is for example `NodeRoles.Role.OVERSEER` and a mode is `NodeRoles.Mode.PREFERRED`.

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;
+    try {
+      List<String> children = zk.listData(path);
+      if (children != null && !children.isEmpty()) {
+        result = new HashMap<>();
+      } else {
+        return depth >= 1 ? Collections.emptyList() : null;
+      }
+      for (String child : children) {
+        Object c = readRecursive(path + "/" + child, zk, depth - 1);
+        result.put(child, c);
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+    }
+    if (depth == 1) {
+      return result.keySet();
+    } else {
+      return result;
+    }
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRole(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    rsp.add("node-roles", Map.of(role,
+            readRecursive(ZkStateReader.NODE_ROLES + "/"+ role,
+                    collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/node/{node}",
+          permission = COLL_READ_PERM)
+  @SuppressWarnings("unchecked")
+  public void rolesForNode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String node = req.getPathTemplateValues().get("node");
+    Map<String, String> ret = new HashMap<String, String>();
+    Map<String, Map<String, Set<String>>> roles = (Map<String, Map<String, Set<String>>>) readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3);
+    for (String role: roles.keySet()) {
+      for (String mode : roles.get(role).keySet()) {
+        if (roles.get(role).get(mode) instanceof List && ((List) roles.get(role).get(mode)).size() == 0) {
+          continue;
+        }
+        Set<String> nodes = roles.get(role).get(mode);
+        if (nodes.contains(node)) ret.put(role, mode);
+      }
+    }
+    for (String role: ret.keySet()) {
+      rsp.add(role, ret.get(role));
+    }
+  }
+
   @EndPoint(method = GET,
-      path = "/cluster/aliases",
-      permission = COLL_READ_PERM)
+          path = "/cluster/node-roles/supported",
+          permission = COLL_READ_PERM)
+  public void supportedRoles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+     Map<String, Object> roleModesSupportedMap = new HashMap<>();
+    for (NodeRoles.Role role: NodeRoles.Role.values()) {
+      roleModesSupportedMap.put(role.toString(), Map.of("modes", role.supportedModes(), "defaultIfAbsent", role.defaultIfAbsent()));
+    }
+    rsp.add("supported-roles", roleModesSupportedMap);
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}/{role-val}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRoleVal(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    String roleVal = req.getPathTemplateValues().get("role-val");
+    List<String> nodes =  collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager()
+            .getDistribStateManager().listData(ZkStateReader.NODE_ROLES + "/"+ role+"/"+roleVal);

Review comment:
       Formatting: please leave a space on each side of the `+` sign.

##########
File path: solr/core/src/java/org/apache/solr/core/NodeRoles.java
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.core;
+
+import java.lang.invoke.MethodHandles;
+import java.util.*;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.StringUtils;
+import org.apache.solr.common.util.StrUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRoles implements MapWriter {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String NODE_ROLES_PROP = "solr.node.roles";
+
+  public static final String ON =  "on";
+  public static final String OFF =  "off";
+  public static final String ALLOWED =  "allowed";
+  public static final String DISALLOWED =  "disallowed";
+  public static final String PREFERRED =  "preferred";
+  public static final Set<String> OVERSEER_MODES = Set.of(ALLOWED, PREFERRED, DISALLOWED);
+  public static final Set<String> ON_OFF = Set.of(ON,OFF);
+
+  public static final String DEFAULT_ROLES_STRING = "data:on,overseer:allowed";
+
+  // Map of roles to mode that are applicable for this node.
+  private Map<Role, String> nodeRoles;
+
+  public NodeRoles(String rolesString) {
+    Map<Role, String> roles = new EnumMap<>(Role.class);
+    if (StringUtils.isEmpty(rolesString)) {
+     rolesString = DEFAULT_ROLES_STRING;
+    }
+    List<String> rolesList = StrUtils.splitSmart(rolesString, ',');
+    for (String s: rolesList) {
+      List<String> roleMode =  StrUtils.splitSmart(s,':');
+      Role r = Role.getRole(roleMode.get(0));
+      if (r.supportedModes().contains(roleMode.get(1))) {
+        roles.put(r, roleMode.get(1));
+      } else {
+        throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+      }
+    }
+    for(Role r: Role.values()) {
+      if (!roles.containsKey(r)) {
+        roles.put(r, r.defaultIfAbsent());
+      }
+    }
+    nodeRoles = Collections.unmodifiableMap(roles);
+  }
+
+  public Map<Role, String> getRoles() {
+    return nodeRoles;
+  }
+
+  public String getRoleMode(Role role) {
+    return nodeRoles.get(role);
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+  }
+
+  public boolean isOverseerAllowed() {

Review comment:
       Name is misleading given `ALLOWED` is a mode. Method should rather be called `isOverseerDisallowed()` for example (and callers changed to add negation), or `isOverseerAllowedOrPreferred()` etc.

##########
File path: solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
##########
@@ -57,92 +64,194 @@
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
 import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
-/** All V2 APIs that have  a prefix of /api/cluster/
- *
+/**
+ * All V2 APIs that have  a prefix of /api/cluster/
  */
 public class ClusterAPI {
   private final CollectionsHandler collectionsHandler;
   private final ConfigSetsHandler configSetsHandler;
 
-  public  final Commands commands = new Commands();
-  public  final ConfigSetCommands configSetCommands = new ConfigSetCommands();
+  public final Commands commands = new Commands();
+  public final ConfigSetCommands configSetCommands = new ConfigSetCommands();
 
   public ClusterAPI(CollectionsHandler ch, ConfigSetsHandler configSetsHandler) {
     this.collectionsHandler = ch;
     this.configSetsHandler = configSetsHandler;
   }
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles",
+          permission = COLL_READ_PERM)
+  public void roles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    Map<String, Object> result = new LinkedHashMap<>();
+
+    rsp.add("node-roles", readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3));
+  }
+
+  Object readRecursive(String path, DistribStateManager zk, int depth) {
+    if (depth == 0) return null;
+    Map<String, Object> result = null;
+    try {
+      List<String> children = zk.listData(path);
+      if (children != null && !children.isEmpty()) {
+        result = new HashMap<>();
+      } else {
+        return depth >= 1 ? Collections.emptyList() : null;
+      }
+      for (String child : children) {
+        Object c = readRecursive(path + "/" + child, zk, depth - 1);
+        result.put(child, c);
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+    }
+    if (depth == 1) {
+      return result.keySet();
+    } else {
+      return result;
+    }
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/role/{role}",
+          permission = COLL_READ_PERM)
+  public void nodesWithRole(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String role = req.getPathTemplateValues().get("role");
+    rsp.add("node-roles", Map.of(role,
+            readRecursive(ZkStateReader.NODE_ROLES + "/"+ role,
+                    collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
+  }
+
+  @EndPoint(method = GET,
+          path = "/cluster/node-roles/node/{node}",
+          permission = COLL_READ_PERM)
+  @SuppressWarnings("unchecked")
+  public void rolesForNode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    String node = req.getPathTemplateValues().get("node");
+    Map<String, String> ret = new HashMap<String, String>();
+    Map<String, Map<String, Set<String>>> roles = (Map<String, Map<String, Set<String>>>) readRecursive(ZkStateReader.NODE_ROLES,
+            collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 3);
+    for (String role: roles.keySet()) {
+      for (String mode : roles.get(role).keySet()) {
+        if (roles.get(role).get(mode) instanceof List && ((List) roles.get(role).get(mode)).size() == 0) {
+          continue;
+        }
+        Set<String> nodes = roles.get(role).get(mode);
+        if (nodes.contains(node)) ret.put(role, mode);
+      }
+    }
+    for (String role: ret.keySet()) {
+      rsp.add(role, ret.get(role));
+    }
+  }
+
   @EndPoint(method = GET,
-      path = "/cluster/aliases",
-      permission = COLL_READ_PERM)
+          path = "/cluster/node-roles/supported",
+          permission = COLL_READ_PERM)
+  public void supportedRoles(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+     Map<String, Object> roleModesSupportedMap = new HashMap<>();
+    for (NodeRoles.Role role: NodeRoles.Role.values()) {
+      roleModesSupportedMap.put(role.toString(), Map.of("modes", role.supportedModes(), "defaultIfAbsent", role.defaultIfAbsent()));

Review comment:
       This endpoint is another reason to remove `NodeRoles.DEFAULT_ROLES_STRING`. It might be confusing to return these values from an API call yet the default ends up being different when no roles are defined.

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -103,6 +103,7 @@
   public static final String STATE_TIMESTAMP_PROP = "stateTimestamp";
   public static final String COLLECTIONS_ZKNODE = "/collections";
   public static final String LIVE_NODES_ZKNODE = "/live_nodes";
+  public static final String NODE_ROLES = "/node_roles";

Review comment:
       A comment here should explain the difference between `/node_roles` and `/roles.json` defined further down.

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -357,7 +358,9 @@ public void command() throws SessionExpiredException {
 
                 overseerElector.setup(context);
 
-                overseerElector.joinElection(context, true);
+                if(cc.nodeRoles.isOverseerAllowed()) {

Review comment:
       formatting: please add space after `if`.

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -1086,6 +1099,11 @@ private void createEphemeralLiveNode() throws KeeperException,
     log.info("Register node as live in ZooKeeper:{}", nodePath);
     List<Op> ops = new ArrayList<>(2);
     ops.add(Op.create(nodePath, null, zkClient.getZkACLProvider().getACLsToAdd(nodePath), CreateMode.EPHEMERAL));
+
+    // Create the roles node as well
+   cc.nodeRoles.getRoles().forEach((role, val) -> ops.add(Op.create(ZkStateReader.NODE_ROLES + "/" + role.roleName+ "/"+val +"/"+ nodeName,

Review comment:
       Another example of ZK path assembly that would benefit from a method building that path (see comment line 862)

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -854,6 +857,14 @@ public static void createClusterZkNodes(SolrZkClient zkClient)
       throws KeeperException, InterruptedException, IOException {
     ZkCmdExecutor cmdExecutor = new ZkCmdExecutor(zkClient.getZkClientTimeout());
     cmdExecutor.ensureExists(ZkStateReader.LIVE_NODES_ZKNODE, zkClient);
+    cmdExecutor.ensureExists(ZkStateReader.NODE_ROLES, zkClient);
+    for (NodeRoles.Role role : NodeRoles.Role.values()) {
+      cmdExecutor.ensureExists(ZkStateReader.NODE_ROLES + "/" + role.roleName, zkClient);

Review comment:
       The various paths `ZkStateReader.NODE_ROLES + "/" + role.roleName` and `ZkStateReader.NODE_ROLES + "/" + role.roleName + "/"+v` are used here and in `ClusterAPI`. It would be better to define a method building them so the actual string assembly has to be defined in only once.

##########
File path: solr/core/src/test/org/apache/solr/cloud/NodeRolesTest.java
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.cloud;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Collection;
+import java.util.Collections;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.V2Request;
+import org.apache.solr.client.solrj.response.V2Response;
+import org.apache.solr.core.NodeRoles;
+import org.junit.After;
+import org.junit.Before;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class NodeRolesTest extends SolrCloudTestCase {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @Before
+  public void setupCluster() throws Exception {
+    configureCluster(1)
+            .addConfig("conf", configset("cloud-minimal"))
+            .configure();
+  }
+
+  @After
+  public void tearDownCluster() throws Exception {
+    shutdownCluster();
+  }
+
+  @SuppressWarnings("unchecked")
+  public void testRoleIntegration() throws Exception {
+    JettySolrRunner j0 = cluster.getJettySolrRunner(0);
+    JettySolrRunner j1 = null, j2 = null;

Review comment:
       setting to `null` is not needed.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -243,12 +247,24 @@ private static boolean existCoreName(String coreName, Slice slice) {
       }
     } else {
       nodeList = new ArrayList<>(liveNodes);
+      filterNonDataNodes(zk, nodeList);

Review comment:
       In the if branch above (when a node set is passed), the set should likely be filtered as well, and warnings issued if some requested nodes have data role configured to off.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org