[
https://issues.apache.org/jira/browse/CASSANDRA-5426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13646605#comment-13646605
]
Sylvain Lebresne commented on CASSANDRA-5426:
---------------------------------------------
The approach looks good to me: I definitively like the idea of having a common
message/header for all repair message. Same for breaking down ARS in separate
files.
One thing I'm not sure of is that it seems that when we get an error, we log it
but we doesn't error out the repair session itself. Maybe we should, otherwise
I fear most people won't notice something went wrong.
Also, when we fail, maybe we could send an error message (typically the
exception message) for easier debugging/reporting.
I also wonder if maybe we should have more of a fail-fast policy when there is
errors. For instance, if one node fail it's validation phase, maybe it might be
worth failing right away and let the user re-trigger a repair once he has fixed
whatever was the source of the error, rather than still differencing/syncing
the other nodes (but I admit that both solutions are possible).
Going a bit further, I think we should add 2 messages to interrupt the
validation and sync phase. If only because that could be useful to users if
they need to stop a repair for some reason, but also, if we get an error during
validation from one node, we could use that to interrupt the other nodes and
thus fail fast while minimizing the amount of work done uselessly. But anyway,
I guess that part can be done in a follow up ticket.
Other than that, a few remarks/nits on the refactor.:
- In RepairMessageType, if gossip is any proof, then it could be wise to add
more "FUTURE" type, say 4 or 5 "just in case". As an aside, I tend to not be a
fan of relying on an enum ordinal for serialization since it's extra fragile
(you should not reorder stuffs for instance, which could easily slip by mistake
imo). I personally prefer assigning the ordinal manually (like in
transport.Message.Type for instance) even if that's a bit more verbose.
Anyway, if people like it the way it is, so be it, but wanted to mention it
nonetheless.
- For the hashCode methods (Differencer, NodePair, RepairJobDesc,...), I'd
prefer using guava's Objects.hashcode() (and Objects.equal() for equals() when
there is null).
- Do we really need RepairMessageHeader? What about making RepairMessage a
RepairJobDesc, a RepairMessageType and a body, rather than creating yet another
class?
- In RepairMessage, not sure it's a good idea to allow a {{null}} body,
especially since RepairMessageVerbHander doesn't handle it really. I'd rather
assert it's not {{null}} and assert we do always have a body serializer in
RepairMessage serializer (since that's really a programing error if we don't).
- The code to create the repair messages feels a bit verbose. What about
adding a static helper in RepairMessage:
{noformat}
public static MessageOut<RepairMessage<T>> createMessage(RepairJobDesc desc,
RepairMessageType type, T body);
{noformat}
or even maybe one helper for each RepairMessageType?
- I would move the gossiper/failure registration in ARS.addToActiveSessions.
- I'd remove Validator.rangeToValidate and just inline desc.range.
- Out of curiosity, what do you mean by the TODO in the comment of
Validator.add(). What is there todo typically? Cause MT has some notion of
valid/invalid ranges but that's historical and not used. Validator is really
just a MT builder. So feels to me that mentioning cases 2 and 4 to later say we
don't consider them will be more confusing than helpful for people looking at
the code for the first time. As a side note, I think we could simplify the
hell out of the MerkleTree class, but that's another story.
- For MerkleTree.fullRange, maybe it's time to add it to the MT serializer
rather than restoring it manually, which is ugly and error prone. Aslo, for the
partitioner, let's maybe have MT uses DatabaseDescriptor.getPartitioner()
directly rather than restoring them manually in Differencer.run().
I also noted that we can remove all the old compat stuff since we don't have
backward compatibility issues with repair, but you already told me you had
started doing it :).
> Redesign repair messages
> ------------------------
>
> Key: CASSANDRA-5426
> URL: https://issues.apache.org/jira/browse/CASSANDRA-5426
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Yuki Morishita
> Assignee: Yuki Morishita
> Priority: Minor
> Labels: repair
> Fix For: 2.0
>
>
> Many people have been reporting 'repair hang' when something goes wrong.
> Two major causes of hang are 1) validation failure and 2) streaming failure.
> Currently, when those failures happen, the failed node would not respond back
> to the repair initiator.
> The goal of this ticket is to redesign message flows around repair so that
> repair never hang.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
