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 17:05:37 UTC
[lucene-solr] branch master updated: Revert "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
The following commit(s) were added to refs/heads/master by this push:
new ae6fe8d Revert "Refactor for code clarity, add some comments."
ae6fe8d is described below
commit ae6fe8d8266a03a3d5eb4e3f8e81fafa8f41f9d6
Author: Gus Heck <gu...@apache.org>
AuthorDate: Sun Jun 7 13:02:45 2020 -0400
Revert "Refactor for code clarity, add some comments."
This reverts commit ebd40918 for which I apparently ran the tests one less time than I needed to
---
.../apache/solr/cloud/api/collections/Assign.java | 66 ++++++++--------------
1 file changed, 22 insertions(+), 44 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 2328df5..cfc401d 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;
+ List<String> createNodeList = null;
if (createNodeSet instanceof List) {
createNodeList = (List<String>) createNodeSet;
@@ -341,13 +341,9 @@ public class Assign {
createNodeList = createNodeSet == null ? null : new ArrayList<>(new LinkedHashSet<>(StrUtils.splitSmart((String) createNodeSet, ",", true)));
}
- // 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);
+ HashMap<String, ReplicaCount> nodeNameVsShardCount = getNodeNameVsShardCount(collectionName, clusterState, createNodeList);
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.
@@ -415,17 +411,24 @@ 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 : throwIfAnyNotLive(createNodeList, clusterState)) {
+ for (String s : nodeList) {
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();
@@ -450,19 +453,6 @@ 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
*/
@@ -560,12 +550,10 @@ 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; // can this be empty list?
+ List<String> nodeList = assignRequest.nodes;
+ 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());
@@ -573,28 +561,18 @@ public class Assign {
int i = 0;
List<ReplicaPosition> result = new ArrayList<>();
- for (String aShard : assignRequest.shardNames) {
- for (Map.Entry<Replica.Type, Integer> e : countsPerReplicaType(assignRequest).entrySet()) {
+ 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 (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 {