[
https://issues.apache.org/jira/browse/CASSANDRA-8984?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14491727#comment-14491727
]
Benedict commented on CASSANDRA-8984:
-------------------------------------
bq. I much prefer it
Well, that will teach me to predict review responses in advance!
bq. So final line of questioning to clarify the benefits of this ticket's
approach vs. our current "primitive" alternative:
There are two main things I have aimed to deliver with this, that IMO the naive
approach doesn't:
1. Forcing implementors to use a safe abstraction
* In the naive world of only two states, it is very easy to conflate the
"preparation for commit" with the "commit" step, but this makes partial or
failed rollback likely, as we've seen recently. We need to ensure commit can
happen for all involved objects before we proceed with commit.
* Without the clearly separated cleanup, commit and abort steps, it's very easy
to intertwine cleanup with one or the other states, and end up with improper
cleanup in some scenarios (as we have multiple bugs of recently).
* Without a separate abort step, we can fail to cleanup properly, since cleanup
is different under this scenario (e.g. leaking bloom filters).
* Without imposing the Throwable signatures, we can forget and introduce state
changes that should happen atomically but throw exceptions, leaving us in an
improper state - which we've also seen a lot of recently.
Now, all of these could be achieved with a separate design doc clearly
outlining how these things need to be safely addressed. Or we can mandate
implementing this interface, and it basically solves itself. We haven't solved
this problem properly to date, so a new tack is definitely required IMO. The
interface can further be documented to explain the rationale, and I should
address that if we pursue this.
2. Making those abstractions safely composable
Once the interface is implemented it delivers a benefit that cannot be achieved
without it, though, which is safe and obvious composability. We have many
different state transition objects in flight at once, and when they each have
their own differing semantics, reasoning about how they interact is hard. More
than that, it's easy to _think_ that it's not hard. We've had a lot of bugs
recently from mistaken assumptions or broken reasoning about how multiple of
these subtly differing semantics each interplayed. Often whackamole-like bugs,
where we fix one weird interaction only to induce another. {{SSTableRewriter}}
is a woeful history of little but this. The new abstraction makes that much
less painful, since you only need to ensure the interface is properly
implemented and a group or graph of these objects can easily be composed: any
method call for a state transition is simply propagated on to the same method
in any children/siblings.
There is also some minor safety introduced by explicitly managing correct API
usage, which a binary variable cannot deliver. With a binary variable we have
conflated prepareToCommit with commit (already, in the bookkeeping, so boding
poorly for the implementor of the important state), so we cannot inform when
one of the steps has not been taken. But that is more of a footnote than of
real significance.
bq. The code changes introduced by this ticket don't stop us from making poor
choices in this regard. The 4 methods indicated for the API in this ticket
would be quite at home in a .{{close()}} method.
I don't think that {{commit()}} can be, since if it does not precede
{{close()}} we must {{abort()}}. However either way, we've enforced a
separation of these concepts, even if {{close()}} does combine them for you in
a safe manner. The important goal is that we don't fall into the most common
pitfall of human thought in programming: efficiently conflating concepts,
eliding some of their complexity so that we can address a more complex and
pressing issue, but failing to realise this step has caused us to mess up
badly, if subtly.
bq. Are there cases where failure in SSTRW or SSTW are best-served by us
keeping the node up and rolling back the changes rather than having startup
logic perform cleanup on orphaned/tmp files?
Well, we don't really want users to be bouncing c* to cleanup partial state.
Especially if there is a programmer induced bug that is recurring, we could
quickly exhaust them of disk space and degrade the cluster.
I'll address your other comments once we decide if we want to pursure this
approach.
> Introduce Transactional API for behaviours that can corrupt system state
> ------------------------------------------------------------------------
>
> Key: CASSANDRA-8984
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8984
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Benedict
> Fix For: 2.1.5
>
> Attachments: 8984_windows_timeout.txt
>
>
> As a penultimate (and probably final for 2.1, if we agree to introduce it
> there) round of changes to the internals managing sstable writing, I've
> introduced a new API called "Transactional" that I hope will make it much
> easier to write correct behaviour. As things stand we conflate a lot of
> behaviours into methods like "close" - the recent changes unpicked some of
> these, but didn't go far enough. My proposal here introduces an interface
> designed to support four actions (on top of their normal function):
> * prepareToCommit
> * commit
> * abort
> * cleanup
> In normal operation, once we have finished constructing a state change we
> call prepareToCommit; once all such state changes are prepared, we call
> commit. If at any point everything fails, abort is called. In _either_ case,
> cleanup is called at the very last.
> These transactional objects are all AutoCloseable, with the behaviour being
> to rollback any changes unless commit has completed successfully.
> The changes are actually less invasive than it might sound, since we did
> recently introduce abort in some places, as well as have commit like methods.
> This simply formalises the behaviour, and makes it consistent between all
> objects that interact in this way. Much of the code change is boilerplate,
> such as moving an object into a try-declaration, although the change is still
> non-trivial. What it _does_ do is eliminate a _lot_ of special casing that we
> have had since 2.1 was released. The data tracker API changes and compaction
> leftover cleanups should finish the job with making this much easier to
> reason about, but this change I think is worthwhile considering for 2.1,
> since we've just overhauled this entire area (and not released these
> changes), and this change is essentially just the finishing touches, so the
> risk is minimal and the potential gains reasonably significant.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
