[jira] [Comment Edited] (CASSANDRA-14705) ReplicaLayout follow-up

["Benedict (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-14705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16607440#comment-16607440 ] 

Benedict edited comment on CASSANDRA-14705 at 9/7/18 5:58 PM:
--------------------------------------------------------------

I've tried to split the patch into digestible chunks.  

The initial two commits are just to get most of the changes that touch a lot of files out of the way, to reduce the cognitive burden of review.  They consist of only a handful of IDE refactors (and a fairly safe find/replace of replicaLayout->replicaPlan).  You can probably ignore these commits almost entirely.

The "main refactor" commit has the guts of the work.
* Introduce {{ReplicaPlan}}, made up of
** two {{ReplicaLayout}}, for the {{live}} and {{liveAndDown}} replicas (maybe we will want to lazily build a separate {{downOnly}} in future, but I've kept it simple for now)
** two {{Endpoints}}, containing the candidate replicas and those we intend to contact; there are now comments clearly defining what the contents of each are in any given scenario.  It's probably the case there were some bugs introduced wrt totalEndpoints before this, but I didn't waste time corroborating.
* There are now separate {{ReplicaPlan}} and {{ReplicaLayout}} for each of read/write and token/range, and we enforce only the correct type be provided to each recipient.  {{ReplicaLayout}} only supports {{natural()}} for read operations, so it is impossible to invoke {{pending()}} here, or think you have {{pending()}} nodes when you do not.  <*Open question*: should we disable {{all()}} for read queries, and only support it for writes?  Haven't tried, and might get a little ugly, but might be conceptually cleaner>
* {{ReplicaCollection}}
** I have caved and implemented a {{count}} method, because there is none in {{Iterables}}.   I'm actually wavering on the review feedback I gave to [~aweisberg]'s original version of this patch (to not implement all the {{Iterables}} method equivalents), for two reasons: we now use these _extensively_, and we also have only one implementation, so it could be very cheap to implement e.g. a {{filterLazily}} and {{\{any,all,none\}Match}}
** I have removed {{select()}} because the correct logic did not benefit from this abstraction.  I have, however, renamed the {{keep()}} method in {{Endpoints}} that re-orders the contents to {{select()}}, to make clear the distinction of semantics.

Then there are a few follow up commits to either fix bugs or perform some follow up clean ups.

Looking forward to your feedback.


was (Author: benedict):
I've tried to split the patch into digestible chunks.  

The initial two commits are just to get most of the changes that touch a lot of files out of the way, to reduce the cognitive burden of review.  They consist of only a handful of IDE refactors (and a fairly safe find/replace of replicaLayout->replicaPlan).  You can probably ignore these commits almost entirely.

The "main refactor" commit has the guts of the work.
* Introduce {{ReplicaPlan}}, made up of
** two {{ReplicaLayout}}, for the {{live}} and {{liveAndDown}} replicas (maybe we will want to lazily build a separate {{downOnly}} in future, but I've kept it simple for now)
** two Endpoints, containing the candidate replicas and those we intend to contact; there are now comments clearly defining what the contents of each are in any given scenario.  It's probably the case there were some bugs introduced wrt totalEndpoints before this, but I didn't waste time corroborating.
* There are now separate {{ReplicaPlan}} and {{ReplicaLayout}} for each of read/write and token/range, and we enforce only the correct type be provided to each recipient.  {{ReplicaLayout}} only supports {{natural()}} for read operations, so it is impossible to invoke {{pending()}} here, or think you have {{pending()}} nodes when you do not.  <*Open question*: should we disable {{all()}} for read queries, and only support it for writes?  Haven't tried, and might get a little ugly, but might be conceptually cleaner>
* ReplicaCollection
** I have caved and implemented a {{count}} method, because there is none in {{Iterables}}.   I'm actually revisiting the review feedback I gave to {{Ariel}} in his version of the patch to not implement all of these, for two reasons: we now use these _extensively_, and we have only one implementation, so it might be OK to implement a {{filterLazily}} and even an {{{any,all,none}Match}}
** I have removed {{select()}} because the correct logic did not benefit from this abstraction.  I have, however, renamed the {{keep()}} method in {{Endpoints}} that re-orders the contents to {{select()}}, to make clear the distinction of semantics.

Then there are a few follow up commits to either fix bugs or perform some follow up clean ups.

Looking forward to your feedback.

> 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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org