14705 by aweisberg · Pull Request #265 · apache/cassandra

aweisberg · 2018-09-10T18:29:27Z

No description provided.

…l, resulting in timeout instead of overload

…FailureDetector)

…tor and ReadRepair

…cting a ReplicaPlan

…rior replicaplan commit

…ere possible ; remove another type parameter

test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java

aweisberg · 2018-09-10T18:53:11Z

src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java

        // There are simply no extra replicas to speculate.
        // Handle this separately so it can record failed attempts to speculate due to lack of replicas
-        if (replicaPlan.contact().size() >= replicaPlan.liveOnly().all().size())
+        if (replicaPlan.contact().size() == replicaPlan.candidates().size())


So >= is not necessary because contact should be a subset of candidates (or it should be an unavailable)? Is it more robust to use >= in case that doesn't hold due to a mistake or have an assertion for that?

I modified to >= in an earlier version of this patch, and decided it was a silly change and rolled it back. Pretty much by definition, if we select contact from candidates, it should always be <=. I guess we could insert an assertion, but at some point we have to drawn the line on what we verify and what we do not. I'm willing to put an assertion in, but I feel it is unnecessary, given we produce almost all contact via a call to candidates.filter

aweisberg · 2018-09-10T19:22:40Z

src/java/org/apache/cassandra/service/reads/repair/BlockingReadRepair.java

    public UnfilteredPartitionIterators.MergeListener getMergeListener(P replicaPlan)
    {
-        return new PartitionIteratorMergeListener<>(replicaPlan, command, this.replicaPlan.consistencyLevel(), this);
+        // TODO: why are we referencing a different replicaPlan here?


TODO indeed looking at DataResolver it's modifying the replica plan without updating the shared one https://github.com/apache/cassandra/pull/265/files#diff-7e5dd130632299911e49b12afe86c85aR121
So they would be different?

I think that was a stale comment. The merging never modified the replicaPlan, and it should never be modified during the SRP. Logically, it should be a snapshot of only the relevant replicas for the SRP.

OK, I think I understand, the original replica plan contacted nodes that never responded and weren't part of the read repair. It's a new replica plan that only includes the nodes that responded.. The consistency level happens to be unchanged across both of them. It would be less misdirecting if it fetched the consistency level from the plan that it is given.

aweisberg · 2018-09-10T19:32:51Z

src/java/org/apache/cassandra/locator/ReplicaPlan.java

+     * we progressively modify via various forms of speculation (initial speculation, rr-read and rr-write)
+     * @param <P>
+     */
+    public static class Shared<P extends ReplicaPlan<?, ?, ?>>


This is kind of a weird place it end up in terms of mutability and immutability. It's not thread safe and if it's supposed to always be referencing the latest instance why immutable?

I'm not sure I really understand the question or criticism. Could it be mutable? For starters, it would require a lot of other changes to ReplicaPlan and ReplicaCollection, that don't seem warranted for this single use case. Besides that, yes, but this would be less threadsafe (this version is actually perfectly safe to have multiple readers, as we do, they just may not see the latest 'contacts' value promptly). I'd be happy to upgrade it to fully threadsafe if it would make us happier, but this was changed to non-threadsafe specifically in response to prior feedback about the old code that it unnecessarily used volatile. It would be trivial to make it fully threadsafe, and the cost would actually be fairly low, and while I'm fairly sure it isn't needed, it would be easier to reason about.

aweisberg · 2018-09-10T19:52:40Z

src/java/org/apache/cassandra/locator/ReplicaLayout.java


        @Override
-        public ForRange withSelected(EndpointsForRange newSelected)
+        public AbstractBounds<PartitionPosition> range()


This seems to get copied around but is unused?

It wasn't unused, but it can be made so - and it is now. But for consistency with ReplicaLayout.ForTokenRead, I think it's fairly harmless.

A wider question is whether we actually want a ReplicaLayout for reads at all. They're only a single Endpoints, and I do wonder if it's worth it. But, again, for consistency with ReplicaPlan I think it's a very small price to pay.

aweisberg · 2018-09-10T21:08:07Z

src/java/org/apache/cassandra/locator/ReplicaPlan.java

+        public SharedForRangeRead(ForRangeRead replicaPlan) { this.replicaPlan = replicaPlan; }
+        public void addToContact(Replica replica) { replicaPlan = replicaPlan.withContact(Endpoints.append(replicaPlan.contact(), replica)); }
+        public ForRangeRead get() { return replicaPlan; }
+        public ForRangeRead getWithContact(EndpointsForRange newContact) { return replicaPlan.withContact(newContact); }


Wait, so it's not shared now? We made a new one without updating the reference?

This is a getter for the case where we don't want to update the reference. ~~I will remove it to avoid any ambiguity.~~ It was introduced so that we could eliminate a lot of type parameters - the withContacts() methods are all now on the most concrete types, so in order to provide one here, we need to implement it per Shared. I will comment to this effect.

aweisberg · 2018-09-10T21:08:22Z

src/java/org/apache/cassandra/locator/ReplicaPlan.java

+    {
+        private ForRangeRead replicaPlan;
+        public SharedForRangeRead(ForRangeRead replicaPlan) { this.replicaPlan = replicaPlan; }
+        public void addToContact(Replica replica) { replicaPlan = replicaPlan.withContact(Endpoints.append(replicaPlan.contact(), replica)); }


addToContact or addContact, grammar seems odd.

It does; in general the non-pluralised form of contact doesn't work well with the method names. Perhaps I should just pluralise and be done with it? Not sure why I was averse to this.

aweisberg · 2018-09-10T21:57:04Z

src/java/org/apache/cassandra/locator/ReplicaLayout.java

+     * See {@link ReplicaLayout#haveWriteConflicts}
+     * @return a 'pending' replica collection, that has had its conflicts with natural repaired
+     */
+    public static <E extends Endpoints<E>> E resolveWriteConflictsInPending(E natural, E pending)


Do these need to be public?

Not anymore

…e paxos replicaPlan, to make clear for paxos, not read/write

src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java

src/java/org/apache/cassandra/service/reads/DigestResolver.java

aweisberg · 2018-09-12T20:55:13Z

src/java/org/apache/cassandra/service/reads/ReadCallback.java

-    // TODO: move to replica layout as well?
-    final int blockfor;
-    final L replicaLayout;
+    final int blockFor; // TODO: move to replica plan as well?


Move it or keep it?

aweisberg · 2018-09-12T20:55:41Z

src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java

    }

    /**
+     * TODO: this method is brittle for its purpose of deciding when we should fail a query;


Should this be a JIRA rather than a TODO or just get done?

aweisberg · 2018-09-12T20:58:12Z

src/java/org/apache/cassandra/batchlog/BatchlogManager.java


-            EndpointsForToken replicas = StorageService.instance.getNaturalAndPendingReplicasForToken(ks, tk);
-            Replicas.temporaryAssertFull(replicas); // TODO in CASSANDRA-14549
+            // TODO: this logic could do with revisiting at some point, as it is unclear what its rationale is


JIRA vs this TODO?

belliottsmith added 19 commits September 7, 2018 12:33

[rR]eplicaLayout->[rR]eplicaPlan, selected() -> contact()

949ec9e

assureSufficientLiveNodes -> assureSufficientReplicas

8190c79

main refactor

70d615f

remove get(Live)?(Sorted)?Natural(AndPending)?Replicas

4523000

fix speculation->rr_extra_read->rr_extra_write

e384d18

cleanup BatchLog.sendSingleReplayMutation

f4eb2d2

fix: could speculate to nodes that would not increase consistencyLeve…

d3eb71c

…l, resulting in timeout instead of overload

initialise Mutable HashMap with capacity

f59dfa8

circleci

14fc69f

comment nit

d45a68a

remove unused ForRangeWrite

b49a088

comments

aab19a9

ensure consistent application of predicates whose value varies (e.g. …

a9aa8dc

…FailureDetector)

ReplicaPlan.Shared, for sharing a ReplicaPlan between Resolver, Execu…

13af81d

…tor and ReadRepair

stop using ReplicaLayout inside ReplicaPlan - only use it for constru…

71669f0

…cting a ReplicaPlan

fix infinite recursion in AbstractReadExecutor.replicaPlan; cleanup p…

75cb98e

…rior replicaplan commit

remove ReplicaPlan.with{Contact,CL}, move to more concrete classes wh…

23235b4

…ere possible ; remove another type parameter

fix unnecessary test breakage

a06fef7

some comments

c80a064

aweisberg commented Sep 10, 2018

View reviewed changes

test/unit/org/apache/cassandra/locator/ReplicaCollectionTest.java Outdated Show resolved Hide resolved

aweisberg commented Sep 10, 2018

View reviewed changes

belliottsmith added 3 commits September 11, 2018 13:34

comments; remove unnecessary subListing in ReplicaPlansforRead; renam…

5a6b3cb

…e paxos replicaPlan, to make clear for paxos, not read/write

more comments

c5d3100

clean up class order / whitespace

2bf7687

aweisberg commented Sep 11, 2018

View reviewed changes

src/java/org/apache/cassandra/service/reads/AbstractReadExecutor.java Outdated Show resolved Hide resolved

aweisberg commented Sep 11, 2018

View reviewed changes

src/java/org/apache/cassandra/service/reads/DigestResolver.java Outdated Show resolved Hide resolved

belliottsmith force-pushed the 14705 branch from df088e9 to 298adf9 Compare September 12, 2018 07:06

ariel's review feedback

7b7d002

belliottsmith force-pushed the 14705 branch from 298adf9 to 7b7d002 Compare September 12, 2018 07:17

belliottsmith added 2 commits September 12, 2018 10:02

alex review feedback

4fa8bd3

tweak

5c3681c

aweisberg commented Sep 12, 2018

View reviewed changes

belliottsmith added 2 commits September 13, 2018 08:25

Ariel's review feedback

26ec44f

More Ariel review feedback

3005687

belliottsmith force-pushed the trunk branch from 822ea57 to 16ef9ac Compare February 8, 2019 17:46

iamaleksey force-pushed the trunk branch from 532c033 to 0d48503 Compare June 12, 2019 15:27

belliottsmith force-pushed the trunk branch from d0be441 to 7dc5b70 Compare August 20, 2019 15:11

smiklosovic closed this Mar 16, 2022

Conversation

aweisberg commented Sep 10, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

belliottsmith Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aweisberg Sep 10, 2018 •

edited

Loading

belliottsmith Sep 12, 2018 •

edited

Loading