[GitHub] [cassandra] yifan-c commented on a change in pull request #954: CASSANDRA-16545: Cluster topology change may produce false unavailable for queries

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #954:
URL: https://github.com/apache/cassandra/pull/954#discussion_r611308786



##########
File path: src/java/org/apache/cassandra/locator/ReplicaPlan.java
##########
@@ -41,10 +45,11 @@
     //      ==> live.all()  (if consistencyLevel.isDCLocal(), then .filter(consistencyLevel.isLocal))
     private final E contacts;
 
-    ReplicaPlan(Keyspace keyspace, ConsistencyLevel consistencyLevel, E contacts)
+    ReplicaPlan(Keyspace keyspace, AbstractReplicationStrategy replicationStrategy, ConsistencyLevel consistencyLevel, E contacts)

Review comment:
       Some call-sites require to pass the `ReplicationStrategy` snapshot explicit, as it uses the snapshot to select the peers to contact, e.g. `org.apache.cassandra.locator.ReplicaPlans#forRead`
   The other call-sites can safely and implicitly just pass the `keyspace` and get the latest RS in the constructor. 
   
   One approach is to provide the both public constructor overrides. 
   
   I tried it earlier, but it is confusing to users, IMO. Because it hides the side effect (i.e. calling the wrong constructor can lead to race). I'd favor explicitness. The client might need to write a bit more code, but easier for later maintenance. 




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org