cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua McKenzie (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-8984) Introduce Transactional API for behaviours that can corrupt system state
Date Fri, 10 Apr 2015 17:48:13 GMT


Joshua McKenzie commented on CASSANDRA-8984:

V2 gets rid of a lot of the no-op duplication and perception of square-peg-round-hole feeling
that bothered me with V1, and while there's boilerplate it helps clarify that the transition
between states alone is important. Along with that, collecting the implementations within
a class that extends AbstractTransactional serves to clarify the relationship between those
methods and the state transitions. While the underlying reality of what the code did in V1
is the same, the structure didn't communicate it in the manner V2 does - I much prefer it.

Notes and nits:
* Unify variable name for TxnProxy:
** SSTableWriter TxnProxy instance named txnproxy
** SequentialWriter TxnProxy instance named txnProxy
* In
throw new IllegalStateException("Commit commit without preceding preparation to commit");
* Consider renaming TxnProxy member classes to TransactionalProxy. Member variable names as
txnProxy is clear enough, but transactional != transaction and shortening the class name doesn't
really buy us anything.

So final line of questioning to clarify the benefits of this ticket's approach vs. our current
"primitive" alternative:
# Why not continue down our current path, implement AutoClosable on these classes, have a
flag we flip internally when they've successfully done what they need to do, and abort() and
raise an appropriate exception when we close() w/out being flagged as completed?
# 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
# What specific benefits does the approach on this ticket give us that boolean flag ->
AutoClosable -> abort() does not?

bq. As things stand we conflate a lot of behaviours into methods like "close" 
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()

> Introduce Transactional API for behaviours that can corrupt system state
> ------------------------------------------------------------------------
>                 Key: CASSANDRA-8984
>                 URL:
>             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

View raw message