[
https://issues.apache.org/jira/browse/CASSANDRA-10726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16068995#comment-16068995
]
Blake Eggleston commented on CASSANDRA-10726:
---------------------------------------------
Sorry for the delay getting to this. I have some comments, but conceptually,
this seems good.
Here's my first round of comments:
{{ConsistencyLevel#isQuorum}}
* a name like {{satisfiesQuorumFor}} would be more descriptive
* needs unit tests that demonstrates it works as expected for different
rf/cl combos. It might be easier to have this call an {{satisfiedQuorumFor(int
quorum)}}, and test that.
{{FBUtilities#waitOnFuturesNanos}}
* this can be removed, it’s not used anywhere
{{DataResolver}}
* {{repairResponseRequestMap}}
** unless you need to mutate this as part of a test, this
should remain private. For testing purposes, you should have a getter method
that makes a defensive copy
* {{responseCntSnapshot}}
** I’m pretty sure you can get rid of this class member. In the
resolve method, sources.length == responseCntSnapshot, so you can just use
RepairMergeListener.sources.length instead of setting a variable on the class.
* {{timeOuts}}
** I think the intended behavior is to wait for a max of
{{timeToWaitNanos}}, but return immediately if any of the read repair
recipients have responded. This method waits for {{timeToWaitNanos}} for each
AsyncOneResponse, so if the last response has been received, it will still wait
for each preceding response future before getting to it and returning.
** should also have a more descriptive name like
{{awaitRepairResponses}} or something
* {{waitRepairToFinishWithPossibleRetry}}
** this method needs a detailed javadoc explaining what sort of
guarantees it’s providing with regard to read repair. Specifically, the idea of
a “monotonic read” is brought up in it’s comments, this has been discussed in
this JIRA, but it also needs to be explained in the code. How it provides that
guarantee without unnecessary blocking should also be explained
{{DataResolverTest}}
* You’re using the wrong brace style for some of the tests still
* I don’t see any tests confirming that the method actually returns as
expected if you receive a read repair response from the second node.
Also, could you please also fix these more general issues before posting
changes:
* fix the compilation errors (it won’t build for me)
* squash and rebase onto trunk, the way the various commits are interleaved
with other commits makes it difficult to review this without the help of github.
* remove the unnecessary uses of the {{final}} keyword. It's used in a lot of
local variables and method arguments. {{final}} typically isn't used in C*
outside of class members
> Read repair inserts should not be blocking
> ------------------------------------------
>
> Key: CASSANDRA-10726
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10726
> Project: Cassandra
> Issue Type: Improvement
> Components: Coordination
> Reporter: Richard Low
> Assignee: Xiaolong Jiang
> Fix For: 3.0.x
>
>
> Today, if there’s a digest mismatch in a foreground read repair, the insert
> to update out of date replicas is blocking. This means, if it fails, the read
> fails with a timeout. If a node is dropping writes (maybe it is overloaded or
> the mutation stage is backed up for some other reason), all reads to a
> replica set could fail. Further, replicas dropping writes get more out of
> sync so will require more read repair.
> The comment on the code for why the writes are blocking is:
> {code}
> // wait for the repair writes to be acknowledged, to minimize impact on any
> replica that's
> // behind on writes in case the out-of-sync row is read multiple times in
> quick succession
> {code}
> but the bad side effect is that reads timeout. Either the writes should not
> be blocking or we should return success for the read even if the write times
> out.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
