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
commit 1328ae1db874865ae44c28e3ac0c94d2f634a23b Author: Noble Paul <[email protected]> AuthorDate: Wed Dec 22 23:19:52 2021 +1100 addressed review comments --- .../apache/solr/cloud/api/collections/Assign.java | 32 ++++++++-------------- .../src/java/org/apache/solr/cluster/Cluster.java | 1 + .../placement/impl/PlacementRequestImpl.java | 2 +- .../impl/SimpleClusterAbstractionsImpl.java | 15 +++++++++- .../src/java/org/apache/solr/core/NodeRoles.java | 17 +++++------- .../placement/ClusterAbstractionsForTest.java | 5 ++++ 6 files changed, 39 insertions(+), 33 deletions(-) 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 7a4394f..88271a1 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 @@ -21,20 +21,7 @@ import static org.apache.solr.common.cloud.ZkStateReader.CORE_NAME_PROP; import java.io.IOException; import java.lang.invoke.MethodHandles; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Locale; -import java.util.Map; -import java.util.Objects; -import java.util.Random; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; import org.apache.solr.client.solrj.cloud.DistribStateManager; @@ -248,22 +235,25 @@ public class Assign { Collections.shuffle(nodeList, random); } } else { - nodeList = new ArrayList<>(liveNodes); - filterNonDataNodes(zk, nodeList); + nodeList = new ArrayList<>(filterNonDataNodes(zk, liveNodes)); Collections.shuffle(nodeList, random); } return nodeList; } - public static void filterNonDataNodes(DistribStateManager zk, List<String> liveNodes) { + public static Collection<String> filterNonDataNodes(DistribStateManager zk, Collection<String> liveNodes) { try { List<String> noData = ClusterAPI.getNodesByRole(NodeRoles.Role.DATA, NodeRoles.Mode.OFF, zk); - if (!noData.isEmpty()) { - liveNodes.removeAll(noData); - } + if (noData.isEmpty()) { + return liveNodes; + } else { + liveNodes = new HashSet<>(liveNodes); + liveNodes.removeAll(noData); + return liveNodes; + } } catch (Exception e) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Problem fetching roles from Zookeeper", e); + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error fetching roles from Zookeeper", e); } } diff --git a/solr/core/src/java/org/apache/solr/cluster/Cluster.java b/solr/core/src/java/org/apache/solr/cluster/Cluster.java index 3b7bdd4..4cd32a4 100644 --- a/solr/core/src/java/org/apache/solr/cluster/Cluster.java +++ b/solr/core/src/java/org/apache/solr/cluster/Cluster.java @@ -31,6 +31,7 @@ public interface Cluster { */ Set<Node> getLiveNodes(); + Set<Node> getLiveNodes(boolean filterNonDataNodes) ; /** * Returns info about the given collection if one exists. * diff --git a/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java b/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java index ff3f090..3814eb6 100644 --- a/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java +++ b/solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java @@ -86,7 +86,7 @@ public class PlacementRequestImpl implements PlacementRequest { throw new Assign.AssignmentException("Bad assign request: empty list of nodes for collection " + solrCollection.getName()); } } else { - nodes = cluster.getLiveNodes(); + nodes = cluster.getLiveNodes(true); if (nodes.isEmpty()) { throw new Assign.AssignmentException("Impossible assign request: no live nodes for collection " + solrCollection.getName()); } diff --git a/solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java b/solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java index 48f3f50..64c0b64 100644 --- a/solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java +++ b/solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java @@ -23,6 +23,7 @@ import java.util.stream.Collectors; import com.google.common.collect.Maps; import org.apache.solr.client.solrj.cloud.SolrCloudManager; +import org.apache.solr.cloud.api.collections.Assign; import org.apache.solr.cluster.*; import org.apache.solr.common.cloud.ClusterState; import org.apache.solr.common.cloud.DocCollection; @@ -53,10 +54,16 @@ class SimpleClusterAbstractionsImpl { static class ClusterImpl implements Cluster { private final Set<Node> liveNodes; + private final Set<Node> liveNodesWithData; private final ClusterState clusterState; ClusterImpl(SolrCloudManager solrCloudManager) throws IOException { - liveNodes = NodeImpl.getNodes(solrCloudManager.getClusterStateProvider().getLiveNodes()); + Set<String> liveNodes = solrCloudManager.getClusterStateProvider().getLiveNodes(); + Collection<String> liveNodesWithData = Assign.filterNonDataNodes( solrCloudManager.getDistribStateManager(), liveNodes); + this.liveNodes = NodeImpl.getNodes(liveNodes); + this.liveNodesWithData = liveNodesWithData.size() == liveNodes.size() ? + this.liveNodes : + NodeImpl.getNodes(liveNodesWithData); clusterState = solrCloudManager.getClusterStateProvider().getClusterState(); } @@ -65,6 +72,12 @@ class SimpleClusterAbstractionsImpl { return liveNodes; } + public Set<Node> getLiveNodes(boolean filterNonDataNodes) { + return filterNonDataNodes ? + liveNodesWithData : + liveNodes; + } + @Override public SolrCollection getCollection(String collectionName) { return SolrCollectionImpl.createCollectionFacade(clusterState, collectionName); 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 714d8b1..317f771 100644 --- a/solr/core/src/java/org/apache/solr/core/NodeRoles.java +++ b/solr/core/src/java/org/apache/solr/core/NodeRoles.java @@ -18,7 +18,6 @@ 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.cloud.ZkStateReader; @@ -61,7 +60,7 @@ public class NodeRoles { } for(Role r: Role.values()) { if (!roles.containsKey(r)) { - roles.put(r, r.defaultIfAbsent()); + roles.put(r, r.defaultMode()); } } nodeRoles = Collections.unmodifiableMap(roles); @@ -100,7 +99,7 @@ public class NodeRoles { return Set.of(Mode.ON, Mode.OFF); } @Override - public Mode defaultIfAbsent() { + public Mode defaultMode() { return Mode.OFF; } }, @@ -110,7 +109,7 @@ public class NodeRoles { return Set.of(Mode.ALLOWED, Mode.PREFERRED, Mode.DISALLOWED); } @Override - public Mode defaultIfAbsent() { + public Mode defaultMode() { return Mode.DISALLOWED; } }; @@ -122,12 +121,10 @@ public class NodeRoles { } public static Role getRole(String value) { - try { - Role role = Role.valueOf(value.toUpperCase(Locale.ROOT)); - return role; - } catch (IllegalArgumentException ex) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value); + for (Role role: Role.values()) { + if (value.equals(role.roleName)) return role; } + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unknown role: " + value); } public abstract Set<Mode> supportedModes(); @@ -135,7 +132,7 @@ public class NodeRoles { /** * Default mode for a role in nodes where this role is not specified. */ - public abstract Mode defaultIfAbsent(); + public abstract Mode defaultMode(); @Override public String toString() { diff --git a/solr/core/src/test/org/apache/solr/cluster/placement/ClusterAbstractionsForTest.java b/solr/core/src/test/org/apache/solr/cluster/placement/ClusterAbstractionsForTest.java index 1260a10..bb85bd3 100644 --- a/solr/core/src/test/org/apache/solr/cluster/placement/ClusterAbstractionsForTest.java +++ b/solr/core/src/test/org/apache/solr/cluster/placement/ClusterAbstractionsForTest.java @@ -43,6 +43,11 @@ class ClusterAbstractionsForTest { } @Override + public Set<Node> getLiveNodes(boolean filterNonDataNodes) { + return liveNodes; + } + + @Override public SolrCollection getCollection(String collectionName) { return collections.get(collectionName); }
