SOLR-16995: Associate with each replica type a property for "numReplicas" #2039

pvcnt · 2023-10-24T08:31:39Z

https://issues.apache.org/jira/browse/SOLR-16995

Description

I am resuming refactoring started in #1928 and #1981 with the goal of simplifying/centralising code related to replica types. The end goal would be to avoid having if (replicaType == Replica.Type.*) scattered across the codebase, and to make it easier to add new replica types.

Solution

Each replica type now has a property name to specify its number of replicas in requests attached to it (nrtReplicas, tlogReplicas and pullReplicas). It removes almost all usages of NRT_REPLICAS, TLOG_REPLICAS and PULL_REPLICAS constants, except in code around v2 APIs.

Tests

New tests have been added for the new methods.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

This allows to avoid repetitive code extracting number of replicas from a message.

pvcnt · 2023-10-24T09:32:08Z

solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java

@@ -118,9 +188,17 @@ public Replica.Type getLeaderType() {
            + "), there must be at least one leader-eligible replica");
  }

-  /** Validate that there is at least one replica that can be a leader. */
-  public void validate() {


This kind of logic should not be in solrj client, moved it in core.

pvcnt · 2023-10-24T09:32:47Z

solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java

+    return fromMessage(message, collection, null);
+  }
+
+  public static ReplicaCount fromMessage(


This should factor all of the existing use cases of creating ReplicaCount, including with a replicationFactor and with a default replica type.

I think it would be useful to have two additional tests in ReplicaCountTest for fromMessage:

The fallback to collection default replication factor works when no replicas of the default type are specified (or no replicas at all are specified),

When no replicas are specified at all (message is empty), the default replica type count gets assigned correctly to the default replica type.

I forgot to answer this comment. I had a look at the suggestions and found out that:

It would be supported but can not happen in practice. When a collection is passed to pull default values from, it will have a replication factor and number of replicas of each type. I don't see a use case where it would have only a replication factor and no number of replicas.

This does not happen in fromMessage, but it handled by the caller (e.g., CreateCollectionCmd#getNumReplicas).

murblanc

Thanks Vincent for doing this well needed cleanup and refactoring.
Overall it looks good to me. I like the approach.

I did leave a few comments on changes to do (or at least to discuss). Nothing really structuring, more like cosmetics and surface changes.

This PR has ben lying around for a while now, I plan to merge it quickly.

murblanc · 2023-11-28T19:00:14Z

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. */


Wouldn't these two methods numReplicasProperties and leaderEligibleReplicaTypes belong in Replica.Type?

Yes, but the reason for not doing it was to avoid exposing more code in solrj while it is only used in Solr itself.

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.

I agree with @murblanc . It's really tiny amounts of code any way.

I really would prefer not to bloat Replica.Type with 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 named CollectionHandlingUtils makes 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.

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 enum put them in Replica, but move them close to the code they're related to.

They do not belong there because they are not used in solrj, but in solr-core.

murblanc · 2023-11-28T19:05:26Z

solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java

    /**
     * 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;


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()).

But on the other it would require yet another look in Replica.Type#defaultType() or static caching. Have you seen other places where it would be useful? If it's only in CollectionHandlingUtils#makeCollectionPropsAndDefaults, I would prefer not to optimise now for a single use case.

Not optimization. Intent is to improve code readability

Not sure of the readability improvement between x.default or x == Type.defaultValue(). The former is more concise, but it is a very small improvement imo.

solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java

solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java

solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java

solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java

solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java

solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java

murblanc · 2023-11-28T23:01:06Z

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() {


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)

It's used every time I need to iterate over replica types explicitly defined in this class. A similar behaviour could be obtained by iterating over all Replica.Type#values and calling ReplicaCount#contains, but it seemed more straightforward this way. We could also have a ReplicaCoubt#forEach method... So many ways to implement this!

solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java

dsmiley · 2023-11-30T13:56:37Z

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. */


I agree with @murblanc . It's really tiny amounts of code any way.

solr/core/src/java/org/apache/solr/cloud/api/collections/CreateShardCmd.java

solr/core/src/java/org/apache/solr/handler/admin/api/RestoreCollectionAPI.java

solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java

solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaCount.java

murblanc · 2023-12-08T13:52:54Z

solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java

+     * onto this replica type. This replica type needs to be leader-eligible.
+     */
+    public static Type defaultType() {
+      return NRT;


add assert NRT.leaderEligible; ?

It's weird to do an assert on something that is purely static and cannot be modified at runtime. A unit test would be better if it's important but... we would be in a bad state everywhere if it wasn't, right?

murblanc · 2024-01-18T03:36:04Z

solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java

-      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()));
+      numReplicas.writeProps(propMap);


writeProps() skips adding counts for replicas that are not present in numReplicas. This is a different behavior from previously , on the left we can see all counts were added.
This causes createCoreLessCollection when it iterates over CollectionHandlingUtils.COLLECTION_PROPS_AND_DEFAULTS, to add the backup collection replica counts for the replicas that were not explicitly specified in the restore message.

writeProps() should likely be made to add all replica counts, it is the simplest fix here.
I ran into problems while working on SIP-20 (dealing with non compatible replica types) and found this issue.

@pvcnt

Associate with each replica type a property for "numReplicas"

db8b5f1

This allows to avoid repetitive code extracting number of replicas from a message.

pvcnt commented Oct 24, 2023

View reviewed changes

pvcnt marked this pull request as ready for review October 24, 2023 09:33

pvcnt changed the title ~~Associate with each replica type a property for "numReplicas"~~ SOLR-16995: Associate with each replica type a property for "numReplicas" Oct 25, 2023

murblanc reviewed Nov 28, 2023

View reviewed changes

Vincent Primault added 3 commits November 29, 2023 16:17

address first batch of review comments

ea77376

another batch of review comments

1380d5d

reuse constant

a747f15

dsmiley reviewed Nov 30, 2023

View reviewed changes

move methods around

0b7526f

murblanc reviewed Dec 8, 2023

View reviewed changes

murblanc merged commit 10c09d0 into apache:main Dec 8, 2023
3 checks passed

pvcnt deleted the num-replicas-property branch January 9, 2024 13:50

pvcnt mentioned this pull request Jan 17, 2024

SOLR-16995: Configure follower behaviour for each replica type #2203

Closed

7 tasks

murblanc reviewed Jan 18, 2024

View reviewed changes

pvcnt mentioned this pull request Jan 19, 2024

SOLR-16995: Configure replication behaviour for each replica type #2207

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16995: Associate with each replica type a property for "numReplicas" #2039

SOLR-16995: Associate with each replica type a property for "numReplicas" #2039

pvcnt commented Oct 24, 2023 •

edited

Loading

pvcnt Oct 24, 2023

pvcnt Oct 24, 2023

murblanc Dec 8, 2023

pvcnt Dec 11, 2023

murblanc left a comment

murblanc Nov 28, 2023

pvcnt Nov 29, 2023 •

edited

Loading

murblanc Nov 30, 2023

dsmiley Nov 30, 2023

pvcnt Nov 30, 2023

murblanc Dec 8, 2023

pvcnt Dec 8, 2023

murblanc Nov 28, 2023

pvcnt Nov 29, 2023

murblanc Nov 29, 2023

pvcnt Nov 30, 2023

murblanc Nov 28, 2023

pvcnt Nov 29, 2023

dsmiley Nov 30, 2023

murblanc Dec 8, 2023

pvcnt Dec 8, 2023

murblanc Jan 18, 2024

SOLR-16995: Associate with each replica type a property for "numReplicas" #2039

SOLR-16995: Associate with each replica type a property for "numReplicas" #2039

Conversation

pvcnt commented Oct 24, 2023 • edited Loading

Description

Solution

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murblanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvcnt Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvcnt commented Oct 24, 2023 •

edited

Loading

pvcnt Nov 29, 2023 •

edited

Loading