You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by is...@apache.org on 2021/12/22 01:45:09 UTC
[solr] branch jira/SOLR15694 updated: Addressing review feedback
This is an automated email from the ASF dual-hosted git repository.
ishan pushed a commit to branch jira/SOLR15694
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/jira/SOLR15694 by this push:
new 5d94773 Addressing review feedback
5d94773 is described below
commit 5d947736be99dbc14b4fe9cd1e99229e17d45dd8
Author: Ishan Chattopadhyaya <is...@apache.org>
AuthorDate: Wed Dec 22 07:15:00 2021 +0530
Addressing review feedback
---
.../apache/solr/cloud/OverseerNodePrioritizer.java | 10 ++-
.../java/org/apache/solr/cloud/ZkController.java | 14 ++--
.../apache/solr/cloud/api/collections/Assign.java | 4 +-
.../java/org/apache/solr/core/CoreContainer.java | 4 +-
.../src/java/org/apache/solr/core/NodeRoles.java | 93 ++++++++++++++--------
.../java/org/apache/solr/handler/ClusterAPI.java | 40 +++++-----
.../test/org/apache/solr/cloud/NodeRolesTest.java | 2 +-
solr/solr-ref-guide/src/node-roles.adoc | 2 +-
.../apache/solr/common/cloud/ZkStateReader.java | 10 ++-
9 files changed, 110 insertions(+), 69 deletions(-)
diff --git a/solr/core/src/java/org/apache/solr/cloud/OverseerNodePrioritizer.java b/solr/core/src/java/org/apache/solr/cloud/OverseerNodePrioritizer.java
index 4bd21dd..db162a1 100644
--- a/solr/core/src/java/org/apache/solr/cloud/OverseerNodePrioritizer.java
+++ b/solr/core/src/java/org/apache/solr/cloud/OverseerNodePrioritizer.java
@@ -73,7 +73,15 @@ public class OverseerNodePrioritizer {
}
}
- overseerDesignates.addAll(ClusterAPI.getNodesByRole(NodeRoles.Role.OVERSEER, NodeRoles.PREFERRED, new ZkDistribStateManager(zkStateReader.getZkClient())));
+ List<String> preferredOverseers = ClusterAPI.getNodesByRole(NodeRoles.Role.OVERSEER, NodeRoles.Mode.PREFERRED,
+ new ZkDistribStateManager(zkStateReader.getZkClient()));
+ for (String preferred: preferredOverseers) {
+ if (overseerDesignates.contains(preferred)) {
+ log.warn("Node " + preferred + " has been configured to be a preferred overseer using both ADDROLE API command " +
+ "as well as using Node Roles (i.e. -Dsolr.node.roles start up property). Only the latter is recommended.");
+ }
+ }
+ overseerDesignates.addAll(preferredOverseers);
if (overseerDesignates.isEmpty()) return;
String ldr = OverseerTaskProcessor.getLeaderNode(zk);
if(overseerDesignates.contains(ldr)) return;
diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkController.java b/solr/core/src/java/org/apache/solr/cloud/ZkController.java
index ee0c87f..8688bfd 100644
--- a/solr/core/src/java/org/apache/solr/cloud/ZkController.java
+++ b/solr/core/src/java/org/apache/solr/cloud/ZkController.java
@@ -358,7 +358,7 @@ public class ZkController implements Closeable {
overseerElector.setup(context);
- if(cc.nodeRoles.isOverseerAllowed()) {
+ if (cc.nodeRoles.isOverseerAllowedOrPreferred()) {
overseerElector.joinElection(context, true);
}
}
@@ -858,10 +858,10 @@ public class ZkController implements Closeable {
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);
- for (String v : role.supportedModes()) {
- cmdExecutor.ensureExists(ZkStateReader.NODE_ROLES + "/" + role.roleName + "/"+v, zkClient);
+ for (NodeRoles.Role role: NodeRoles.Role.values()) {
+ cmdExecutor.ensureExists(NodeRoles.getZNodeForRole(role), zkClient);
+ for (NodeRoles.Mode mode: role.supportedModes()) {
+ cmdExecutor.ensureExists(NodeRoles.getZNodeForRoleMode(role, mode), zkClient);
}
}
@@ -924,7 +924,7 @@ public class ZkController implements Closeable {
ElectionContext context = new OverseerElectionContext(zkClient,
overseer, getNodeName());
overseerElector.setup(context);
- if(cc.nodeRoles.isOverseerAllowed()) {
+ if (cc.nodeRoles.isOverseerAllowedOrPreferred()) {
overseerElector.joinElection(context, false);
}
}
@@ -1101,7 +1101,7 @@ public class ZkController implements Closeable {
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,
+ cc.nodeRoles.getRoles().forEach((role, mode) -> ops.add(Op.create(NodeRoles.getZNodeForRoleMode(role, mode) +"/" + nodeName,
null, zkClient.getZkACLProvider().getACLsToAdd(nodePath), CreateMode.EPHEMERAL)));
zkClient.multi(ops, true);
diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
index c5aef60..209539b 100644
--- a/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
+++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
@@ -256,8 +256,8 @@ public class Assign {
public static void filterNonDataNodes(DistribStateManager zk, List<String> liveNodes) {
try {
- List<String> noData = ClusterAPI.getNodesByRole(NodeRoles.Role.DATA, NodeRoles.OFF, zk);
- if(!noData.isEmpty()){
+ List<String> noData = ClusterAPI.getNodesByRole(NodeRoles.Role.DATA, NodeRoles.Mode.OFF, zk);
+ if (!noData.isEmpty()) {
liveNodes.removeAll(noData);
}
} catch (Exception e) {
diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java
index 80d356f..ef55b51 100644
--- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java
+++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java
@@ -946,9 +946,9 @@ public class CoreContainer {
});
clusterSingletons.setReady();
- if (NodeRoles.PREFERRED.equals(nodeRoles.getRoleMode(NodeRoles.Role.OVERSEER))) {
+ if (NodeRoles.Mode.PREFERRED.equals(nodeRoles.getRoleMode(NodeRoles.Role.OVERSEER))) {
try {
- log.info("This node is started as a preferred overseer");
+ log.info("This node has been started as a preferred overseer");
zkSys.getZkController().setPreferredOverseer();
} catch (KeeperException | InterruptedException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, e);
diff --git a/solr/core/src/java/org/apache/solr/core/NodeRoles.java b/solr/core/src/java/org/apache/solr/core/NodeRoles.java
index 71c0303..714d8b1 100644
--- a/solr/core/src/java/org/apache/solr/core/NodeRoles.java
+++ b/solr/core/src/java/org/apache/solr/core/NodeRoles.java
@@ -21,30 +21,29 @@ 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.cloud.ZkStateReader;
import org.apache.solr.common.util.StrUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class NodeRoles implements MapWriter {
+public class NodeRoles {
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 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 PREFERRED = "preferred";*/
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;
+ private Map<Role, Mode> nodeRoles;
public NodeRoles(String rolesString) {
- Map<Role, String> roles = new EnumMap<>(Role.class);
+ Map<Role, Mode> roles = new EnumMap<>(Role.class);
if (StringUtils.isEmpty(rolesString)) {
rolesString = DEFAULT_ROLES_STRING;
}
@@ -52,10 +51,12 @@ public class NodeRoles implements MapWriter {
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));
+ Mode m = Mode.valueOf(roleMode.get(1).toUpperCase(Locale.ROOT));
+ if (r.supportedModes().contains(m)) {
+ roles.put(r, m);
} else {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role mode: " + roleMode.get(0));
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Unknown role mode '" + roleMode.get(1) + "' for role '" + r + "'");
}
}
for(Role r: Role.values()) {
@@ -66,34 +67,51 @@ public class NodeRoles implements MapWriter {
nodeRoles = Collections.unmodifiableMap(roles);
}
- public Map<Role, String> getRoles() {
+ public Map<Role, Mode> getRoles() {
return nodeRoles;
}
- public String getRoleMode(Role role) {
+ public Mode getRoleMode(Role role) {
return nodeRoles.get(role);
}
- @Override
- public void writeMap(EntryWriter ew) {
- nodeRoles.forEach((role, s) -> ew.putNoEx(role.roleName, s));
+ public boolean isOverseerAllowedOrPreferred() {
+ Mode roleMode = nodeRoles.get(Role.OVERSEER);
+ return Mode.ALLOWED.equals(roleMode) || Mode.PREFERRED.equals(roleMode);
}
- public boolean isOverseerAllowed() {
- String roleMode = nodeRoles.get(Role.OVERSEER);
- return ALLOWED.equals(roleMode) || PREFERRED.equals(roleMode);
- }
+ public enum Mode {
+ ON, OFF, ALLOWED, PREFERRED, DISALLOWED;
+
+ @Override
+ /**
+ * Need this lowercasing so that the ZK references use the lowercase form, which is
+ * also the form documented in user facing documentation.
+ */
+ public String toString() {
+ return name().toLowerCase(Locale.ROOT);
+ }
+ };
public enum Role {
- DATA("data"),
+ DATA("data") {
+ @Override
+ public Set<Mode> supportedModes() {
+ return Set.of(Mode.ON, Mode.OFF);
+ }
+ @Override
+ public Mode defaultIfAbsent() {
+ return Mode.OFF;
+ }
+ },
OVERSEER("overseer") {
@Override
- public Set<String> supportedModes() {
- return OVERSEER_MODES;
+ public Set<Mode> supportedModes() {
+ return Set.of(Mode.ALLOWED, Mode.PREFERRED, Mode.DISALLOWED);
}
@Override
- public String defaultIfAbsent() {
- return DISALLOWED;
+ public Mode defaultIfAbsent() {
+ return Mode.DISALLOWED;
}
};
@@ -104,26 +122,33 @@ public class NodeRoles implements MapWriter {
}
public static Role getRole(String value) {
- for (Role role: Role.values()) {
- if (value.equals(role.roleName)) return role;
+ try {
+ Role role = Role.valueOf(value.toUpperCase(Locale.ROOT));
+ return role;
+ } catch (IllegalArgumentException ex) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value);
}
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value);
}
- public Set<String> supportedModes() {
- return ON_OFF;
- }
+ public abstract Set<Mode> supportedModes();
/**
* Default mode for a role in nodes where this role is not specified.
*/
- public String defaultIfAbsent() {
- return OFF;
- }
+ public abstract Mode defaultIfAbsent();
@Override
public String toString() {
- return super.toString().toLowerCase(Locale.ROOT);
+ return roleName;
}
}
+
+ public static String getZNodeForRole(Role role) {
+ return ZkStateReader.NODE_ROLES + "/" + role.roleName;
+ }
+
+ public static String getZNodeForRoleMode(Role role, Mode mode) {
+ return ZkStateReader.NODE_ROLES + "/" + role.roleName + "/" + mode;
+ }
+
}
diff --git a/solr/core/src/java/org/apache/solr/handler/ClusterAPI.java b/solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
index 6a0148e..dce5ea5 100644
--- a/solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
+++ b/solr/core/src/java/org/apache/solr/handler/ClusterAPI.java
@@ -20,6 +20,7 @@ package org.apache.solr.handler;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.util.*;
+import java.util.stream.Collectors;
import com.google.common.collect.Maps;
import org.apache.solr.api.Command;
@@ -85,23 +86,21 @@ public class ClusterAPI {
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;
+ Map<String, Object> result;
try {
List<String> children = zk.listData(path);
if (children != null && !children.isEmpty()) {
result = new HashMap<>();
} else {
- return depth >= 1 ? Collections.emptyList() : null;
+ return depth >= 1 ? Collections.emptySet(): null;
}
- for (String child : children) {
+ for (String child: children) {
Object c = readRecursive(path + "/" + child, zk, depth - 1);
result.put(child, c);
}
@@ -121,7 +120,7 @@ public class ClusterAPI {
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,
+ readRecursive(ZkStateReader.NODE_ROLES + "/" + role,
collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager().getDistribStateManager(), 2)));
}
@@ -135,10 +134,8 @@ public class ClusterAPI {
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;
- }
+ for (String mode: roles.get(role).keySet()) {
+ if (roles.get(role).get(mode).isEmpty()) continue;
Set<String> nodes = roles.get(role).get(mode);
if (nodes.contains(node)) ret.put(role, mode);
}
@@ -154,30 +151,33 @@ public class ClusterAPI {
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()));
+ roleModesSupportedMap.put(role.toString(),
+ Map.of("modes", (role.supportedModes()).stream().map(NodeRoles.Mode::toString).collect(Collectors.toList())));
}
rsp.add("supported-roles", roleModesSupportedMap);
}
@EndPoint(method = GET,
- path = "/cluster/node-roles/role/{role}/{role-val}",
+ path = "/cluster/node-roles/role/{role}/{mode}",
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");
+ public void nodesWithRoleMode(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+ // Here, deal with raw strings instead of Role & Mode types so as to handle roles and modes
+ // that are not understood by this node (possibly during a rolling upgrade)
+ String roleStr = req.getPathTemplateValues().get("role");
+ String modeStr = req.getPathTemplateValues().get("mode");
+
List<String> nodes = collectionsHandler.getCoreContainer().getZkController().getSolrCloudManager()
- .getDistribStateManager().listData(ZkStateReader.NODE_ROLES + "/"+ role+"/"+roleVal);
- rsp.add( "node-roles", Map.of(role, Collections.singletonMap(roleVal, nodes)));
+ .getDistribStateManager().listData(ZkStateReader.NODE_ROLES + "/" + roleStr + "/" + modeStr);
+ rsp.add( "node-roles", Map.of(roleStr, Collections.singletonMap(modeStr, nodes)));
}
- public static List<String> getNodesByRole(NodeRoles.Role role, String val, DistribStateManager zk)
+ public static List<String> getNodesByRole(NodeRoles.Role role, NodeRoles.Mode mode, DistribStateManager zk)
throws InterruptedException, IOException, KeeperException {
try {
- return zk.listData(ZkStateReader.NODE_ROLES+"/"+ role+ "/"+val);
+ return zk.listData(ZkStateReader.NODE_ROLES + "/" + role + "/" + mode);
} catch (NoSuchElementException e) {
return Collections.emptyList();
}
-
}
diff --git a/solr/core/src/test/org/apache/solr/cloud/NodeRolesTest.java b/solr/core/src/test/org/apache/solr/cloud/NodeRolesTest.java
index 63f7d86..0fc0e95 100644
--- a/solr/core/src/test/org/apache/solr/cloud/NodeRolesTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/NodeRolesTest.java
@@ -52,7 +52,7 @@ public class NodeRolesTest extends SolrCloudTestCase {
@SuppressWarnings("unchecked")
public void testRoleIntegration() throws Exception {
JettySolrRunner j0 = cluster.getJettySolrRunner(0);
- JettySolrRunner j1 = null, j2 = null;
+ JettySolrRunner j1, j2;
V2Response rsp = new V2Request.Builder("/cluster/node-roles/supported").GET().build().process(cluster.getSolrClient());
Map<String, Object> l = (Map<String, Object>) rsp._get("supported-roles", Collections.emptyMap());
assertTrue(l.containsKey("data"));
diff --git a/solr/solr-ref-guide/src/node-roles.adoc b/solr/solr-ref-guide/src/node-roles.adoc
index e5a70ec..455de14 100644
--- a/solr/solr-ref-guide/src/node-roles.adoc
+++ b/solr/solr-ref-guide/src/node-roles.adoc
@@ -45,7 +45,7 @@ Examples: `-Dsolr.node.roles=data:on,overseer:allowed` or `-Dsolr.node.roles=ove
[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.
+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 accommodate the functionality associated with these roles.
====
.Supported roles
diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
index a01cab2..8814cc9 100644
--- a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
+++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
@@ -103,7 +103,16 @@ public class ZkStateReader implements SolrCloseable {
public static final String STATE_TIMESTAMP_PROP = "stateTimestamp";
public static final String COLLECTIONS_ZKNODE = "/collections";
public static final String LIVE_NODES_ZKNODE = "/live_nodes";
+
+ /**
+ * The following, node_roles and roles.json are for assigning roles to
+ * nodes. The node_roles is the preferred way (using -Dsolr.node.roles param),
+ * and roles.json is used by legacy ADDROLE API command.
+ * TODO: Deprecate & remove support for roles.json in an upcoming release.
+ */
public static final String NODE_ROLES = "/node_roles";
+ public static final String ROLES = "/roles.json";
+
public static final String ALIASES = "/aliases.json";
/**
* This ZooKeeper file is no longer used starting with Solr 9 but keeping the name around to check if it
@@ -125,7 +134,6 @@ public class ZkStateReader implements SolrCloseable {
public static final String TLOG_REPLICAS = "tlogReplicas";
public static final String READ_ONLY = "readOnly";
- public static final String ROLES = "/roles.json";
public static final String CONFIGS_ZKNODE = "/configs";
public final static String CONFIGNAME_PROP = "configName";