SOLR-16995: Associate with each replica type a property for "numReplicas" #2039
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,6 @@ | |
|
|
||
| package org.apache.solr.cloud.api.collections; | ||
|
|
||
| import static org.apache.solr.common.params.CollectionAdminParams.ALIAS; | ||
| import static org.apache.solr.common.params.CollectionAdminParams.COLL_CONF; | ||
| import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDREPLICA; | ||
|
|
@@ -139,7 +135,6 @@ public void call(ClusterState clusterState, ZkNodeProps message, NamedList<Objec | |
| // fail fast if parameters are wrong or incomplete | ||
| List<String> shardNames = populateShardNames(message, router); | ||
| ReplicaCount numReplicas = getNumReplicas(message); | ||
|
|
||
| DocCollection newColl = null; | ||
| final String collectionPath = DocCollection.getCollectionPath(collectionName); | ||
|
|
@@ -590,12 +585,22 @@ public static List<String> populateShardNames(ZkNodeProps message, String router | |
| return shardNames; | ||
| } | ||
|
|
||
| private ReplicaCount getNumReplicas(ZkNodeProps message) { | ||
| ReplicaCount numReplicas = ReplicaCount.fromMessage(message); | ||
| boolean hasLeaderEligibleReplica = numReplicas.hasLeaderReplica(); | ||
| if (!hasLeaderEligibleReplica && !numReplicas.contains(Replica.Type.defaultType())) { | ||
| // Ensure that there is at least one replica that can become leader if the user did | ||
| // not force a replica count. | ||
| numReplicas.put(Replica.Type.defaultType(), 1); | ||
| } else if (!hasLeaderEligibleReplica) { | ||
| // This can still fail if the user manually forced "0" replica counts. | ||
|
There was a problem hiding this comment. We could simplify by removing the BTW the There was a problem hiding this comment. This slightly changes the existing behaviour. As noted in the comment, if a user explicitly passes There was a problem hiding this comment. And yes There was a problem hiding this comment. I think if user passes only 0 counts for all replicas it's ok to set the default replica count to 1. There was a problem hiding this comment. The current behaviour is:
What you are suggesting is to change the third situation, and instead to silently transform the user request into There was a problem hiding this comment. Well, then it works because if you pass There was a problem hiding this comment. This succeeds: ``` There was a problem hiding this comment. Yes, but this fails: curl -X POST http://localhost:8983/api/collections -H 'Content-Type: application/json' -d '
{
"name": "techproducts_v2",
"config": "_default",
"numShards": 1,
"createReplicas": false,
"nrtReplicas":0
}'with: {"responseHeader":{"status":400,"QTime":28},"error":{"metadata":{"error-class":"org.apache.solr.common.SolrException","root-error-class":"org.apache.solr.common.SolrException"},"msg":"nrtReplicas + tlogReplicas must be greater than 0","code":400}}There was a problem hiding this comment. It may be a bug (what is the purpose of validating the number of replicas if not creating any replicas?), but the goal of this PR is not to change this behaviour. There was a problem hiding this comment. It's a small change in behavior for an edge-case. Wouldn't break any existing user. |
||
| throw new SolrException( | ||
| SolrException.ErrorCode.BAD_REQUEST, | ||
| "Unexpected number of replicas (" | ||
| + numReplicas | ||
| + "), there must be at least one leader-eligible replica"); | ||
| } | ||
| return numReplicas; | ||
| } | ||
|
|
||
| String getConfigName(String coll, ZkNodeProps message) throws IOException { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't these two methods
numReplicasPropertiesandleaderEligibleReplicaTypesbelong inReplica.Type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the reason for not doing it was to avoid exposing more code in solrj while it is only used in Solr itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But I'd still value more a cleaner object oriented design vs packaging/deployment concerns. SolrJ exposes a lot of internals to clients so within that framework I think it makes sense to group code related to the same concepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @murblanc . It's really tiny amounts of code any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really would prefer not to bloat
Replica.Typewith methods, especially static ones, that are not used there. Since those methods are used only from API/Command layer, I believe that having them in a class namedCollectionHandlingUtilsmakes sense. And since they are static (this is a different story for non-static methods, I added one as requested elsewhere in this PR) I do not see the difference it makes to have them here or there. So I definitely prefer not adding random util methods to a place where they do not belong.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you say "do not belong".
Except standard Java classes, these two static methods depend only on
Replica.Type.If you don't want to put them in the
enumput them inReplica, but move them close to the code they're related to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do not belong there because they are not used in solrj, but in solr-core.