murblanc commented on code in PR #2039: URL: https://github.com/apache/solr/pull/2039#discussion_r1408434915
########## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ########## @@ -97,9 +94,12 @@ public class CreateCollectionCmd implements CollApiCmds.CollectionApiCommand { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); private final CollectionCommandContext ccc; + private final EnumSet<Replica.Type> leaderEligibleReplicaTypes; public CreateCollectionCmd(CollectionCommandContext ccc) { this.ccc = ccc; + leaderEligibleReplicaTypes = CollectionHandlingUtils.leaderEligibleReplicaTypes(); + ; Review Comment: spare semicolon ########## solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java: ########## @@ -135,6 +140,20 @@ public class CollectionHandlingUtils { } } + /** Returns names of properties that are used to specify a number of replicas of a given type. */ Review Comment: Wouldn't these two methods `numReplicasProperties` and `leaderEligibleReplicaTypes` belong in `Replica.Type`? ########## solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java: ########## @@ -103,31 +104,41 @@ public enum Type { * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can become a leader. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - NRT(true), + NRT(true, CollectionAdminParams.NRT_REPLICAS), /** * Writes to transaction log, but not to index, uses replication. Any {@link Type#TLOG} replica * can become leader (by first applying all local transaction log elements). If a replica is of * type {@link Type#TLOG} but is also the leader, it will behave as a {@link Type#NRT}. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - TLOG(true), + TLOG(true, CollectionAdminParams.TLOG_REPLICAS), /** * Doesn’t index or writes to transaction log. Just replicates from {@link Type#NRT} or {@link * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard leaders (i.e., if there * are only pull replicas in the collection at some point, updates will fail same as if there is * no leaders, queries continue to work), so they don’t even participate in elections. */ - PULL(false); + PULL(false, CollectionAdminParams.PULL_REPLICAS); public final boolean leaderEligible; + public final String numReplicasProperty; Review Comment: I suggest renaming this into `numReplicasPropertyName` ########## solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java: ########## @@ -103,31 +104,41 @@ public enum Type { * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can become a leader. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - NRT(true), + NRT(true, CollectionAdminParams.NRT_REPLICAS), /** * Writes to transaction log, but not to index, uses replication. Any {@link Type#TLOG} replica * can become leader (by first applying all local transaction log elements). If a replica is of * type {@link Type#TLOG} but is also the leader, it will behave as a {@link Type#NRT}. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - TLOG(true), + TLOG(true, CollectionAdminParams.TLOG_REPLICAS), /** * Doesn’t index or writes to transaction log. Just replicates from {@link Type#NRT} or {@link * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard leaders (i.e., if there * are only pull replicas in the collection at some point, updates will fail same as if there is * no leaders, queries continue to work), so they don’t even participate in elections. */ - PULL(false); + PULL(false, CollectionAdminParams.PULL_REPLICAS); public final boolean leaderEligible; Review Comment: Shall we add a `defaultReplica` final boolean here and set it to true for the replica defined as default (via `defaultType()`). This will will clean up some code needing the default replica type (for example `CollectionHandlingUtils.makeCollectionPropsAndDefaults()`). ########## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java: ########## @@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps message, NamedList<Objec CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, ccc.getZkStateReader()); String async = message.getStr(ASYNC); - ZkNodeProps addReplicasProps = - new ZkNodeProps( + Map<String, Object> addReplicasProps = + Utils.makeMap( COLLECTION_PROP, - collectionName, + (Object) collectionName, SHARD_ID_PROP, sliceName, - ZkStateReader.NRT_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.NRT)), - ZkStateReader.TLOG_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.TLOG)), - ZkStateReader.PULL_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.PULL)), CollectionHandlingUtils.CREATE_NODE_SET, message.getStr(CollectionHandlingUtils.CREATE_NODE_SET), CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.toString(waitForFinalState)); + for (Replica.Type replicaType : numReplicas.keySet()) { Review Comment: Since `ReplicaCount` already has a `fromMessage` method (and `fromProps`), wouldn't it make sense to have it implement a `toMessage` (not sure about the name) method returning a property -> count map, and here we would simply `addAll` its entries to the map? Or, more realistically given we might try to build the message in a `ZkNodeProps` directly (see comment below), have the `toMessage` take as a parameter the target message and add the replica counts there. In other places in SolrCloud a `Map` is used though. Maybe add two methods (or more, after seeing other places in the code using other types, commented there as well) in `ReplicaCount`. The `Map` based one can do the actual work and the other ones call it. ########## solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java: ########## @@ -100,6 +166,10 @@ public int total() { return countByType.values().stream().reduce(Integer::sum).orElse(0); } + public int count(EnumSet<Replica.Type> replicaTypes) { Review Comment: This method is called from two places, and both calls are to get the total of leader eligible replicas, which forces the caller to build the (enum) set first. I suggest specializing this method and renaming it `countLeaderEligible` or something along these lines to simplify the callers. We could even go simpler and have a boolean method `hasLeaderReplicas`. The caller would look even better. ########## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java: ########## @@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps message, NamedList<Objec CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, ccc.getZkStateReader()); String async = message.getStr(ASYNC); - ZkNodeProps addReplicasProps = - new ZkNodeProps( + Map<String, Object> addReplicasProps = + Utils.makeMap( COLLECTION_PROP, - collectionName, + (Object) collectionName, Review Comment: Compiler unhappy without this cast? ########## solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java: ########## @@ -1055,7 +1051,7 @@ public Map<String, Object> execute( DocCollection.verifyProp(m, prop); } if (m.get(REPLICATION_FACTOR) != null) { - m.put(NRT_REPLICAS, m.get(REPLICATION_FACTOR)); + m.put(Replica.Type.defaultType().numReplicasProperty, m.get(REPLICATION_FACTOR)); Review Comment: Unrelated: wonder if we should stop putting `REPLICATION_FACTOR` parameters but keep accepting them if present, to phase this parameter out in a couple of major releases... ########## solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java: ########## @@ -49,6 +49,8 @@ public interface CollectionAdminParams { String REPLICATION_FACTOR = "replicationFactor"; + String REPLICA_TYPE = "type"; Review Comment: There is already a `REPLICA_TYPE` in `ZkStateReader` ########## solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java: ########## @@ -129,17 +129,8 @@ public String getRequestType() { } private static void writeNumReplicas(ReplicaCount numReplicas, ModifiableSolrParams params) { - if (numReplicas.contains(Replica.Type.NRT)) { - params.add( - CollectionAdminParams.NRT_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.NRT))); - } - if (numReplicas.contains(Replica.Type.TLOG)) { - params.add( - CollectionAdminParams.TLOG_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.TLOG))); - } - if (numReplicas.contains(Replica.Type.PULL)) { - params.add( - CollectionAdminParams.PULL_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.PULL))); + for (Replica.Type replicaType : numReplicas.keySet()) { Review Comment: Maybe a third destination type for a `ReplicaCount.toMessage()`? 😢 Do we need to introduce a functional interface to be able to do add's on all these different types from a single method and avoid all these for loops sprinkled over the code? ########## solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java: ########## @@ -18,31 +18,92 @@ import java.util.Arrays; import java.util.EnumMap; +import java.util.EnumSet; import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CollectionAdminParams; /** Tracks the number of replicas per replica type. This class is mutable. */ public final class ReplicaCount { private final EnumMap<Replica.Type, Integer> countByType; - public ReplicaCount(Integer nrtReplicas, Integer tlogReplicas, Integer pullReplicas) { + private ReplicaCount() { this.countByType = new EnumMap<>(Replica.Type.class); - put(Replica.Type.NRT, nrtReplicas); - put(Replica.Type.TLOG, tlogReplicas); - put(Replica.Type.PULL, pullReplicas); } public static ReplicaCount empty() { - return new ReplicaCount(null, null, null); + return new ReplicaCount(); } public static ReplicaCount of(Replica.Type type, Integer count) { - ReplicaCount replicaCount = empty(); + ReplicaCount replicaCount = new ReplicaCount(); replicaCount.put(type, count); return replicaCount; } + public static ReplicaCount of(Integer nrtReplicas, Integer tlogReplicas, Integer pullReplicas) { + ReplicaCount replicaCount = new ReplicaCount(); + replicaCount.put(Replica.Type.NRT, nrtReplicas); + replicaCount.put(Replica.Type.TLOG, tlogReplicas); + replicaCount.put(Replica.Type.PULL, pullReplicas); + return replicaCount; + } + + public static ReplicaCount fromMessage(ZkNodeProps message) { + return fromMessage(message, null); + } + + public static ReplicaCount fromMessage(ZkNodeProps message, DocCollection collection) { + return fromMessage(message, collection, null); + } + + public static ReplicaCount fromMessage( Review Comment: Need Javadoc for this method, it is obscure enough. Also, need javadoc on `fromProps` and an explanation why it is much simpler. ########## solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java: ########## @@ -61,6 +122,11 @@ public boolean contains(Replica.Type type) { return countByType.containsKey(type); } + /** Returns the replica types for which a number of replicas was explicitely defined. */ + public Set<Replica.Type> keySet() { Review Comment: Do we still need this method if this class implements a method to directly count the number of leader replicas or if there's at least one leader replica? (see comment on `count()` below) (doing the review in GitHub not in the IDE so can't easily find all callers) ########## solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java: ########## @@ -129,17 +129,8 @@ public String getRequestType() { } private static void writeNumReplicas(ReplicaCount numReplicas, ModifiableSolrParams params) { - if (numReplicas.contains(Replica.Type.NRT)) { - params.add( - CollectionAdminParams.NRT_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.NRT))); - } - if (numReplicas.contains(Replica.Type.TLOG)) { - params.add( - CollectionAdminParams.TLOG_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.TLOG))); - } - if (numReplicas.contains(Replica.Type.PULL)) { - params.add( - CollectionAdminParams.PULL_REPLICAS, String.valueOf(numReplicas.get(Replica.Type.PULL))); + for (Replica.Type replicaType : numReplicas.keySet()) { Review Comment: Maybe a third destination type for a `ReplicaCount.toMessage()`? 😢 Do we need to introduce a functional interface to be able to do add's on all these different types from a single method and avoid all these for loops sprinkled over the code? ########## solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java: ########## @@ -103,17 +104,19 @@ public class ReindexCollectionCmd implements CollApiCmds.CollectionApiCommand { public static final String STATE = "state"; public static final String PHASE = "phase"; - private static final List<String> COLLECTION_PARAMS = - Arrays.asList( - ZkStateReader.CONFIGNAME_PROP, - ZkStateReader.NUM_SHARDS_PROP, - ZkStateReader.NRT_REPLICAS, - ZkStateReader.PULL_REPLICAS, - ZkStateReader.TLOG_REPLICAS, - ZkStateReader.REPLICATION_FACTOR, - "shards", - CollectionAdminParams.CREATE_NODE_SET_PARAM, - CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM); + private static final List<String> COLLECTION_PARAMS = makeCollectionParams(); + + private static List<String> makeCollectionParams() { + List<String> collectionParams = new ArrayList<>(10); + collectionParams.add(ZkStateReader.CONFIGNAME_PROP); + collectionParams.add(ZkStateReader.NUM_SHARDS_PROP); + collectionParams.add(ZkStateReader.REPLICATION_FACTOR); + collectionParams.add("shards"); + collectionParams.add(CollectionAdminParams.CREATE_NODE_SET_PARAM); + collectionParams.add(CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM); + collectionParams.addAll(CollectionHandlingUtils.numReplicasProperties()); + return List.copyOf(collectionParams); Review Comment: Why copy instead of returning `collectionParams`? ########## solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java: ########## @@ -341,10 +318,10 @@ private void createCoreLessCollection( propMap.put(Overseer.QUEUE_OPERATION, CREATE.toString()); // mostly true. Prevents autoCreated=true in the collection state. propMap.put("fromApi", "true"); - propMap.put(REPLICATION_FACTOR, numReplicas.get(Replica.Type.NRT)); - propMap.put(NRT_REPLICAS, numReplicas.get(Replica.Type.NRT)); - propMap.put(TLOG_REPLICAS, numReplicas.get(Replica.Type.TLOG)); - propMap.put(PULL_REPLICAS, numReplicas.get(Replica.Type.PULL)); + propMap.put(REPLICATION_FACTOR, numReplicas.get(Replica.Type.defaultType())); + for (Replica.Type replicaType : numReplicas.keySet()) { Review Comment: Here too we might be able to use here one of the ReplicaCount.toMessage methods? ########## solr/core/src/java/org/apache/solr/cloud/api/collections/ReindexCollectionCmd.java: ########## @@ -338,14 +339,8 @@ public void call(ClusterState clusterState, ZkNodeProps message, NamedList<Objec if (rf != null) { propMap.put(ZkStateReader.REPLICATION_FACTOR, rf); } - if (numNrt != null) { - propMap.put(ZkStateReader.NRT_REPLICAS, numNrt); - } - if (numTlog != null) { - propMap.put(ZkStateReader.TLOG_REPLICAS, numTlog); - } - if (numPull != null) { - propMap.put(ZkStateReader.PULL_REPLICAS, numPull); + for (Replica.Type replicaType : numReplicas.keySet()) { Review Comment: Maybe we can use here one of the `ReplicaCount.toMessage` methods I suggest adding? ########## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ########## @@ -590,12 +589,22 @@ public static List<String> populateShardNames(ZkNodeProps message, String router return shardNames; } - private static ReplicaCount getNumReplicas(ZkNodeProps message) { - int numTlogReplicas = message.getInt(TLOG_REPLICAS, 0); - int numNrtReplicas = - message.getInt( - NRT_REPLICAS, message.getInt(REPLICATION_FACTOR, numTlogReplicas > 0 ? 0 : 1)); - return new ReplicaCount(numNrtReplicas, numTlogReplicas, message.getInt(PULL_REPLICAS, 0)); + private ReplicaCount getNumReplicas(ZkNodeProps message) { + ReplicaCount numReplicas = ReplicaCount.fromMessage(message); + int numLeaderEligibleReplicas = numReplicas.count(leaderEligibleReplicaTypes); + if (numLeaderEligibleReplicas == 0 && !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 (numLeaderEligibleReplicas == 0) { + // This can still fail if the user manually forced "0" replica counts. Review Comment: We could simplify by removing the `else` branch here and keeping only the `numLeaderEligibleReplicas == 0` in the `if` condition, basically adding a default replica (assumed to be leader eligible) if the user passed 0 leaders in the request. BTW the `numReplicas.validate()` call in existing code (line 142) was useless since upon return from `getNumReplicas` there was always at least one leader replica. ########## solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java: ########## @@ -103,31 +104,41 @@ public enum Type { * support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can become a leader. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - NRT(true), + NRT(true, CollectionAdminParams.NRT_REPLICAS), /** * Writes to transaction log, but not to index, uses replication. Any {@link Type#TLOG} replica * can become leader (by first applying all local transaction log elements). If a replica is of * type {@link Type#TLOG} but is also the leader, it will behave as a {@link Type#NRT}. A shard * leader will forward updates to all active {@link Type#NRT} and {@link Type#TLOG} replicas. */ - TLOG(true), + TLOG(true, CollectionAdminParams.TLOG_REPLICAS), /** * Doesn’t index or writes to transaction log. Just replicates from {@link Type#NRT} or {@link * Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard leaders (i.e., if there * are only pull replicas in the collection at some point, updates will fail same as if there is * no leaders, queries continue to work), so they don’t even participate in elections. */ - PULL(false); + PULL(false, CollectionAdminParams.PULL_REPLICAS); public final boolean leaderEligible; + public final String numReplicasProperty; - Type(boolean b) { - this.leaderEligible = b; + Type(boolean leaderEligible, String numReplicasProperty) { + this.leaderEligible = leaderEligible; + this.numReplicasProperty = numReplicasProperty; } public static Type get(String name) { return StrUtils.isNullOrEmpty(name) ? NRT : Type.valueOf(name.toUpperCase(Locale.ROOT)); } + + /** + * Returns a default replica type. It is most notably used by the replica factor, which maps + * onto this replica type. Review Comment: Maybe add a comment that this replica needs to be leader eligible ########## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java: ########## @@ -97,33 +101,30 @@ public void call(ClusterState clusterState, ZkNodeProps message, NamedList<Objec CollectionHandlingUtils.waitForNewShard(collectionName, sliceName, ccc.getZkStateReader()); String async = message.getStr(ASYNC); - ZkNodeProps addReplicasProps = - new ZkNodeProps( + Map<String, Object> addReplicasProps = + Utils.makeMap( COLLECTION_PROP, - collectionName, + (Object) collectionName, SHARD_ID_PROP, sliceName, - ZkStateReader.NRT_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.NRT)), - ZkStateReader.TLOG_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.TLOG)), - ZkStateReader.PULL_REPLICAS, - String.valueOf(numReplicas.get(Replica.Type.PULL)), CollectionHandlingUtils.CREATE_NODE_SET, message.getStr(CollectionHandlingUtils.CREATE_NODE_SET), CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.toString(waitForFinalState)); + for (Replica.Type replicaType : numReplicas.keySet()) { + addReplicasProps.put(replicaType.numReplicasProperty, numReplicas.get(replicaType)); + } - Map<String, Object> propertyParams = new HashMap<>(); - CollectionHandlingUtils.addPropertyParams(message, propertyParams); - addReplicasProps = addReplicasProps.plus(propertyParams); - if (async != null) addReplicasProps.getProperties().put(ASYNC, async); + CollectionHandlingUtils.addPropertyParams(message, addReplicasProps); + if (async != null) { + addReplicasProps.put(ASYNC, async); + } final NamedList<Object> addResult = new NamedList<>(); try { new AddReplicaCmd(ccc) .addReplica( clusterState, - addReplicasProps, + new ZkNodeProps(addReplicasProps), Review Comment: We should avoid copying maps if possible. Could we build the message in a `ZkNodeProps` like existing code does. ########## solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java: ########## @@ -70,15 +69,17 @@ @Path("/backups/{backupName}/restore") public class RestoreCollectionAPI extends BackupAPIBase { - private static final Set<String> CREATE_PARAM_ALLOWLIST = - Set.of( - COLL_CONF, - REPLICATION_FACTOR, - NRT_REPLICAS, - TLOG_REPLICAS, - PULL_REPLICAS, - CREATE_NODE_SET_PARAM, - CREATE_NODE_SET_SHUFFLE_PARAM); + private static final Set<String> CREATE_PARAM_ALLOWLIST = Set.copyOf(makeCreateParamAllowList()); Review Comment: I wouldn't do a copy. I know in existing code `Set.of()` is unmodifiable, but `CREATE_PARAM_ALLOWLIST` is private and used in a single place. ########## solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementRequestImpl.java: ########## @@ -33,21 +33,6 @@ public class PlacementRequestImpl implements PlacementRequest { private final Set<Node> targetNodes; private final ReplicaCount numReplicas; - @Deprecated Review Comment: Ok to remove since it's internal and not used prior to this PR, but it's not directly related to nor needed by this PR. ########## solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java: ########## @@ -262,12 +258,17 @@ public static Object verifyProp(Map<String, Object> props, String propName) { public static Object verifyProp(Map<String, Object> props, String propName, Object def) { Object o = props.get(propName); - if (o == null) return def; + if (o == null) { + return def; + } + // Properties associated with a number of replicas are parsed as integers. Review Comment: Maybe put the replica type loop in the `default` case of the `switch`, so it's not run for no reason for each property? (I'm assuming the switch is optimized). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org