[
https://issues.apache.org/jira/browse/CASSANDRA-15564?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Blake Eggleston updated CASSANDRA-15564:
----------------------------------------
Status: Changes Suggested (was: Review In Progress)
Finished my first round of review, notes below:
*o.a.c.repair.RepairRunnable*
I like the idea of splitting up the runMayThrow method into a few, more
focused methods. However the way it's done here doesn't make the code easier to
follow or more testable. runMayThrow has basically been cut in half. I think a
better approach would be if runMayThrow was kept intact, but the different
sections were broken out into their own methods. For example, initial table
filtering could have its own method, calculating neighbors and common ranges
could have it's own method, etc, etc. This would make runMayThrow a lot easier
to understand.
* If we added a mutable traceState field to the class, we could also eliminate
the need for the context class.
* The use of {{Either}} should be replaced with a SkipRepairException
throw/catch
*o.a.c.utils.Either*
* We should avoid general purpose 'tuple' classes like this (ie: Pair), and
use more use case specific classes.
* This has a lot of methods and functionality that aren't used anywhere (or
tested)
* I think an exception would generally be a better choice?
*o.a.c.utils.Retry*
* could also use a comment explaining purpose and usage
* this seems to only be used in in-jvm tests, maybe that would be a better
place for it to live?
*Misc questions / nits:*
* I don't understand your comment "Why is this before start when its
publishing the start event? For backwards compatability (...)" what's incorrect
about logging that we're starting the repair command there?
* Usage of {{final}}: we generally don't declare local variables as final. We
used to for closure captured variables, but stopped after java 8 stopped
requiring that.
* Preconditions#checkState is preferable to assertions (ie:
{{RepairRunnable#run}}).
> Refactor repair coordinator so errors are consistent
> ----------------------------------------------------
>
> Key: CASSANDRA-15564
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15564
> Project: Cassandra
> Issue Type: Sub-task
> Components: Consistency/Repair
> Reporter: David Capwell
> Assignee: David Capwell
> Priority: Normal
> Labels: pull-request-available
> Time Spent: 10h 50m
> Remaining Estimate: 0h
>
> This is to split the change in CASSANDRA-15399 so the refactor is isolated
> out.
> Currently the repair coordinator special cases the exit cases at each call
> site; this makes it so that errors can be inconsistent and there are cases
> where proper complete isn't done (proper notifications, and forgetting to
> update ActiveRepairService).
> [Circle
> CI|https://circleci.com/gh/dcapwell/cassandra/tree/bug%2FrepairCoordinatorJmxConsistency]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
