murblanc commented on code in PR #2203: URL: https://github.com/apache/solr/pull/2203#discussion_r1494802432
########## solr/core/src/java/org/apache/solr/handler/component/CloudReplicaSource.java: ########## @@ -288,8 +287,8 @@ public Builder params(SolrParams params) { return this; } - public Builder onlyNrt(boolean onlyNrt) { - this.onlyNrt = onlyNrt; + public Builder onlyRealTime(boolean onlyRealTime) { Review Comment: Do we need an attribute on the replica types that tells if a replica supports real time? Where does the knowledge of if a replica type supports real time come from? ########## solr/core/src/java/org/apache/solr/handler/component/CloudReplicaSource.java: ########## @@ -170,10 +170,9 @@ private List<String> findReplicas( .filter(replica -> replica.isActive(clusterState.getLiveNodes())) .filter( replica -> - !builder.onlyNrt - || (replica.getType() == Replica.Type.NRT - || (replica.getType() == Replica.Type.TLOG - && isShardLeader.test(replica)))) + !builder.onlyRealTime + || (replica.getType().follower && !replica.getType().followerSkipCommit) + || (replica.getType().leaderEligible && isShardLeader.test(replica))) Review Comment: if the replica is shard leader then obviously it is also leader eligible, right? Before the change checking that the type is TLOG was already useless since PULL can't be leader. ########## solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java: ########## @@ -104,27 +104,35 @@ 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, CollectionAdminParams.NRT_REPLICAS), + NRT(true, true, false, 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, CollectionAdminParams.TLOG_REPLICAS), + TLOG(true, true, 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, CollectionAdminParams.PULL_REPLICAS); + PULL(false, false, false, CollectionAdminParams.PULL_REPLICAS); public final boolean leaderEligible; + public final boolean follower; + public final boolean followerSkipCommit; public final String numReplicasPropertyName; - Type(boolean leaderEligible, String numReplicasPropertyName) { + Type( + boolean leaderEligible, + boolean follower, Review Comment: What is the (conceptual) difference between `follower` and `leaderEligible`? (or `follower` and having a transaction log, as introduced in PR2207) What is the meaning of `followerSkipCommit`? I guess we need javadoc for all these attributes. ########## solr/core/src/java/org/apache/solr/handler/component/CloudReplicaSource.java: ########## @@ -170,10 +170,9 @@ private List<String> findReplicas( .filter(replica -> replica.isActive(clusterState.getLiveNodes())) .filter( replica -> - !builder.onlyNrt - || (replica.getType() == Replica.Type.NRT - || (replica.getType() == Replica.Type.TLOG - && isShardLeader.test(replica)))) + !builder.onlyRealTime + || (replica.getType().follower && !replica.getType().followerSkipCommit) Review Comment: This is statement is (in my opinion) harder to understand than the code it is replacing. A replica is considered real time if it can be a follower but the follower doesn't skip commit (whatever that means)? I don't get it. In any case, the logic telling if a replica type is real time or not based on replica type attributes likely belongs in the `Replica.Type` enum, like the other attributes. -- 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