You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by gu...@apache.org on 2020/06/07 16:24:59 UTC
[lucene-solr] 02/03: Refactor for code clarity, add some comments.
This is an automated email from the ASF dual-hosted git repository.
gus pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit ebd409187a78f4c0d21980b46b69fe1ce281b0cb
Author: Gus Heck <gu...@apache.org>
AuthorDate: Sun Jun 7 11:47:17 2020 -0400
Refactor for code clarity, add some comments.
---
.../apache/solr/cloud/api/collections/Assign.java | 66 ++++++++++++++--------
1 file changed, 44 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 cfc401d..2328df5 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<String> createNodeList = null;
+ List<String> createNodeList;
if (createNodeSet instanceof List) {
createNodeList = (List<String>) createNodeSet;
@@ -341,9 +341,13 @@ public class Assign {
createNodeList = createNodeSet == null ? null : new ArrayList<>(new LinkedHashSet<>(StrUtils.splitSmart((String) createNodeSet, ",", true)));
}
- HashMap<String, ReplicaCount> 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.
+ throwIfAnyNotLive(createNodeList,clusterState);
if (createNodeList == null) { // We only care if we haven't been told to put new replicas on specific nodes.
+ HashMap<String, ReplicaCount> nodeNameVsShardCount = getNodeNameVsShardCount(collectionName, clusterState, null);
long availableSlots = 0;
for (Map.Entry<String, ReplicaCount> ent : nodeNameVsShardCount.entrySet()) {
//ADDREPLICA can put more than maxShardsPerNode on an instance, so this test is necessary.
@@ -411,24 +415,17 @@ public class Assign {
static HashMap<String, ReplicaCount> getNodeNameVsShardCount(String collectionName,
ClusterState clusterState, List<String> createNodeList) {
- Set<String> nodes = clusterState.getLiveNodes();
-
- List<String> nodeList = new ArrayList<>(nodes.size());
- nodeList.addAll(nodes);
- if (createNodeList != null) nodeList.retainAll(createNodeList);
-
HashMap<String, ReplicaCount> nodeNameVsShardCount = new HashMap<>();
- for (String s : nodeList) {
+ for (String s : throwIfAnyNotLive(createNodeList, clusterState)) {
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<String, DocCollection> collections = clusterState.getCollectionsMap();
@@ -453,6 +450,19 @@ public class Assign {
return nodeNameVsShardCount;
}
+ private static List<String> throwIfAnyNotLive(List<String> createNodeList, ClusterState clusterState) {
+ if (createNodeList != null) {
+ if (!clusterState.getLiveNodes().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,10 +560,12 @@ public class Assign {
@Override
public List<ReplicaPosition> assign(SolrCloudManager solrCloudManager, AssignRequest assignRequest) throws Assign.AssignmentException, IOException, InterruptedException {
ClusterState clusterState = solrCloudManager.getClusterStateProvider().getClusterState();
- List<String> nodeList = assignRequest.nodes;
+ List<String> nodeList = assignRequest.nodes; // can this be empty list?
- HashMap<String, Assign.ReplicaCount> nodeNameVsShardCount = Assign.getNodeNameVsShardCount(assignRequest.collectionName, clusterState, assignRequest.nodes);
if (nodeList == null || nodeList.isEmpty()) {
+ HashMap<String, Assign.ReplicaCount> 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<Assign.ReplicaCount> sortedNodeList = new ArrayList<>(nodeNameVsShardCount.values());
sortedNodeList.sort(Comparator.comparingInt(Assign.ReplicaCount::weight));
nodeList = sortedNodeList.stream().map(replicaCount -> replicaCount.nodeName).collect(Collectors.toList());
@@ -561,18 +573,28 @@ public class Assign {
int i = 0;
List<ReplicaPosition> result = new ArrayList<>();
- for (String aShard : assignRequest.shardNames)
- for (Map.Entry<Replica.Type, Integer> 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<Replica.Type, Integer> e : countsPerReplicaType(assignRequest).entrySet()) {
for (int j = 0; j < e.getValue(); j++) {
+ // TODO: not sure if we can receive an non-null, empty assingRequest.nodes,
+ // but if we do this appears to result in a div/0. Also not sure if empty should be
+ // same as null or an error (with better message) here.
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<Replica.Type, Integer> 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 {