From b8ff4c76361e1ecddd9466c899ff644d32351375 Mon Sep 17 00:00:00 2001 From: Gus Heck Date: Sat, 13 Jun 2020 12:13:39 -0400 Subject: [PATCH] Refactor for code clarity, add some comments. --- .../solr/cloud/api/collections/Assign.java | 73 +++++++++++++------ 1 file changed, 51 insertions(+), 22 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 cfc401d5ba8..b577340845e 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 @@ -200,7 +200,7 @@ public class Assign { } return defaultValue; } - + private static int defaultCounterValue(DocCollection collection, boolean newCollection) { if (newCollection) return 0; int defaultValue = collection.getReplicas().size(); @@ -332,7 +332,7 @@ public class Assign { , shard, nrtReplicas, tlogReplicas, pullReplicas, createNodeSet); DocCollection coll = clusterState.getCollection(collectionName); int maxShardsPerNode = coll.getMaxShardsPerNode() == -1 ? Integer.MAX_VALUE : coll.getMaxShardsPerNode(); - List createNodeList = null; + List createNodeList; if (createNodeSet instanceof List) { createNodeList = (List) createNodeSet; @@ -341,9 +341,13 @@ public class Assign { createNodeList = createNodeSet == null ? null : new ArrayList<>(new LinkedHashSet<>(StrUtils.splitSmart((String) createNodeSet, ",", true))); } - HashMap nodeNameVsShardCount = getNodeNameVsShardCount(collectionName, clusterState, createNodeList); + // produces clear message when down nodes are the root cause, without this the user just + // gets a log message of detail about the nodes that are up, and a message that policies could not + // be satisfied which then requires study to diagnose the issue. + checkLiveNodes(createNodeList,clusterState); if (createNodeList == null) { // We only care if we haven't been told to put new replicas on specific nodes. + HashMap nodeNameVsShardCount = getNodeNameVsShardCount(collectionName, clusterState, null); long availableSlots = 0; for (Map.Entry ent : nodeNameVsShardCount.entrySet()) { //ADDREPLICA can put more than maxShardsPerNode on an instance, so this test is necessary. @@ -411,24 +415,21 @@ public class Assign { static HashMap getNodeNameVsShardCount(String collectionName, ClusterState clusterState, List createNodeList) { - Set nodes = clusterState.getLiveNodes(); - - List nodeList = new ArrayList<>(nodes.size()); - nodeList.addAll(nodes); - if (createNodeList != null) nodeList.retainAll(createNodeList); - HashMap nodeNameVsShardCount = new HashMap<>(); - for (String s : nodeList) { + List liveNodes = createNodeList == null || createNodeList.isEmpty() ? + new ArrayList<>(clusterState.getLiveNodes()) : + checkLiveNodes(createNodeList, clusterState); + + for (String s : liveNodes) { nodeNameVsShardCount.put(s, new ReplicaCount(s)); } + + // if we were given a list, just use that, don't worry about counts if (createNodeList != null) { // Overrides petty considerations about maxShardsPerNode - if (createNodeList.size() != nodeNameVsShardCount.size()) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "At least one of the node(s) specified " + createNodeList + " are not currently active in " - + nodeNameVsShardCount.keySet() + ", no action taken."); - } return nodeNameVsShardCount; } + + // if we get here we were not given a createNodeList, build a map with real counts. DocCollection coll = clusterState.getCollection(collectionName); int maxShardsPerNode = coll.getMaxShardsPerNode() == -1 ? Integer.MAX_VALUE : coll.getMaxShardsPerNode(); Map collections = clusterState.getCollectionsMap(); @@ -453,6 +454,22 @@ public class Assign { return nodeNameVsShardCount; } + // throw an exception if any node int the supplied list is not live. + // Empty or null list always succeeds and returns the input. + private static List checkLiveNodes(List createNodeList, ClusterState clusterState) { + Set liveNodes = clusterState.getLiveNodes(); + if (createNodeList != null) { + if (!liveNodes.containsAll(createNodeList)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "At least one of the node(s) specified " + createNodeList + " are not currently active in " + + createNodeList + ", no action taken."); + } + // the logic that was extracted to this method used to create a defensive copy but no code + // was modifying the copy, if this method is made protected or public we want to go back to that + } + return createNodeList; // unmodified, but return for inline use + } + /** * Thrown if there is an exception while assigning nodes for replicas */ @@ -550,29 +567,41 @@ public class Assign { @Override public List assign(SolrCloudManager solrCloudManager, AssignRequest assignRequest) throws Assign.AssignmentException, IOException, InterruptedException { ClusterState clusterState = solrCloudManager.getClusterStateProvider().getClusterState(); - List nodeList = assignRequest.nodes; + List nodeList = assignRequest.nodes; // can this be empty list? - HashMap nodeNameVsShardCount = Assign.getNodeNameVsShardCount(assignRequest.collectionName, clusterState, assignRequest.nodes); if (nodeList == null || nodeList.isEmpty()) { + HashMap nodeNameVsShardCount = + Assign.getNodeNameVsShardCount(assignRequest.collectionName, clusterState, nodeList); + // if nodelist was empty, this map will be empty too. (passing null above however gets a full map) ArrayList sortedNodeList = new ArrayList<>(nodeNameVsShardCount.values()); sortedNodeList.sort(Comparator.comparingInt(Assign.ReplicaCount::weight)); nodeList = sortedNodeList.stream().map(replicaCount -> replicaCount.nodeName).collect(Collectors.toList()); } + // otherwise we get a div/0 below + assert !nodeList.isEmpty(); + int i = 0; List result = new ArrayList<>(); - for (String aShard : assignRequest.shardNames) - for (Map.Entry e : ImmutableMap.of(Replica.Type.NRT, assignRequest.numNrtReplicas, - Replica.Type.TLOG, assignRequest.numTlogReplicas, - Replica.Type.PULL, assignRequest.numPullReplicas - ).entrySet()) { + for (String aShard : assignRequest.shardNames) { + for (Map.Entry e : countsPerReplicaType(assignRequest).entrySet()) { for (int j = 0; j < e.getValue(); j++) { result.add(new ReplicaPosition(aShard, j, e.getKey(), nodeList.get(i % nodeList.size()))); i++; } } + } return result; } + + // keeps this big ugly construction block out of otherwise legible code + private ImmutableMap countsPerReplicaType(AssignRequest assignRequest) { + return ImmutableMap.of( + Replica.Type.NRT, assignRequest.numNrtReplicas, + Replica.Type.TLOG, assignRequest.numTlogReplicas, + Replica.Type.PULL, assignRequest.numPullReplicas + ); + } } public static class RulesBasedAssignStrategy implements AssignStrategy {