Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Mon, 20 Apr 2015 19:04:01 +0000 (UTC)
From: "Benedict (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12782710.1426633092000.50846.1429556641723@Atlassian.JIRA>
In-Reply-To: <JIRA.12782710.1426633092000@Atlassian.JIRA>
References: <JIRA.12782710.1426633092000@Atlassian.JIRA>
 <JIRA.12782710.1426633092067@arcas>
Subject: [jira] [Commented] (CASSANDRA-8984) Introduce Transactional API for
 behaviours that can corrupt system state
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/CASSANDRA-8984?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14503434#comment-14503434 ] 

Benedict commented on CASSANDRA-8984:
-------------------------------------

bq. BigTableWriter reaching into the guts of IndexWriter is error-prone 
bq. There's enough usage of finish() and finishAndClose() floating around that it comes off as an undocumented extension

Agreed, these had both vaguely bugged me as well. The first I've fixed and uploaded (along with the missing nits from your past comment). The second I'm not sure how best to address: the problem is that it includes prepareToCommit in the semantics. So we have a few options: 1) some really horrendous generics; 2) moving prepareToCommit into the Transactional, making it no-args, and requiring any commit preparation arguments be provided in a separate method; 3) leaving as-is. 

I think I'm leaning towards (2), though may change my mind once taken through to its conclusion. It isn't perfect, but it does allow us to clearly codify all correct behaviours, at the cost of needing a little use of only-temporary builder-like state inside of some Transactional objects, both for the prepareToCommit parameters, and also for any return values (like in SSTableRewriter, or SSTableWriter, where we return the list of readers, or the reader, respectively).

bq. We may want to convert the touched /io tests to take advantage of and exercise the various writers being Transactional

Yeah. The reader tests probably not, but we should perhaps introduce a special SequentialWriter test that can work on both kinds of implementation to test the behaviours are consistent with Transactional. We appear to not have any kind of SSTableWriter test, either. I think that should be a separate ticket, since its scope is much broader, but perhaps I can introduce a starter touching just this functionality and file a follow-up.

> 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)