[
https://issues.apache.org/jira/browse/CASSANDRA-14705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16611749#comment-16611749
]
Alex Petrov commented on CASSANDRA-14705:
-----------------------------------------
Thank you for the patch. Some comments, all rather minor:
* we could rename {{liveOnly}} to {{live}} unless "only" here bears
* {{allUncontactedCandidates}} could be just {{uncontactedCandidates}}
* {{ForTokenWrite}} can be just {{ForWrite}} since we've removed
{{ForRangeWrite}}, similarly in {{forTokenWriteLiveAndDown}} and
{{forTokenWrite}}
* we can avoid iterating in {{ReplicaPlan#forWrite}} checking for
{{isTransient}} and short-circuit it for non-transient keyspace with
{{CL#hasTransient}}.
* in {{EndpointsFor(Range|Token)}} we have checks for range/token so they
matched for concatenated ranges. Should we apply same checks when constructing
layouts?
* I know it was same before the patch and we can do it in a separate one, but
{{isSufficientReplicasForRead}} and {{assureSufficientReplicas}} share the
logic which potentially makes it prone to modifications just in one place and
not the other. Looks like we could combine the two. In same methods, naming is
either liveReplicas or allLive for same argument.
* {{assureSufficientReplicas}} might need word "live" in it
* {{ReplicaCount}} can now use the new {{count}} method
* {{ReplicaPlans#maybeMerge}} is currently checking for
{{isSufficientReplicasForRead}} and then proceeds with filtering the nodes,
unlike token reads, which filter and then check. Since this is the only place
{{isSufficientReplicasForRead}}, this might be an opportunity for consolidation.
And some that we've discussed offline for history:
* looks like you're half a step away from removing all the logic from
{{ConsistencyLevel}}, which is great. Theoretically, all static methods from
there could be moved to {{ReplicaPlan}} and things like "filterForQuery" could
have stronger invariants, such as {{ForRead<Endpoints<?>>}}, same is true for
{{isSufficientReplicasForRead/write}}, especially is applicable for write since
we have to pass {{Endpoints}} separately there even though they're technically
in the same bucket of {{ForWrite}}.
* we've discussed it offline and it's probably worth to do in a follow-up
ticket, but collecting replicas and {{ReplicaLayouts}} in general might be
worth to cover with tests. They're tested indirectly, however mostly through
dtests. Maybe flushing out some things, especially related to pending nodes
(which are generally hard to test), might be good candidates there.
> ReplicaLayout follow-up
> -----------------------
>
> Key: CASSANDRA-14705
> URL: https://issues.apache.org/jira/browse/CASSANDRA-14705
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Benedict
> Assignee: Benedict
> Priority: Major
>
> Clarify the new {{ReplicaLayout}} code, separating it into ReplicaPlan (for
> what we want to do) and {{ReplicaLayout}} (for what we know about the
> cluster), with well defined semantics (and comments in the rare cases those
> semantics are weird)
> Found and fixed some bugs:
> - {{commitPaxos}} was using only live nodes, when needed to include down
> - We were not writing to pending transient replicas
> - On write, we were not hinting to full nodes with transient
> replication enabled (since we filtered to {{liveOnly}}, in order to include
> our transient replicas above {{blockFor}})
> - If we speculated, in {{maybeSendAdditionalReads}} (in read repair)
> we would only consult the same node we had speculated too. This also applied
> to {{maybeSendAdditionalWrites}} - and this issue was also true pre-TR.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
