[
https://issues.apache.org/jira/browse/CASSANDRA-8568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14550461#comment-14550461
]
Benedict commented on CASSANDRA-8568:
-------------------------------------
I've uploaded two branches for cassci [for
2.2|https://github.com/belliottsmith/cassandra/tree/8568-2.2] and
[trunk|https://github.com/belliottsmith/cassandra/tree/8568-trunk], but the
merge was clean so nothing to worry about from a review perspective. The rebase
was also nearly 100% clean (just a couple of methods that were introduced, that
had to have a couple of arguments updated).
I've fixed all of your nits bar 1: the naming. I'm not by any means wed to the
naming I've used, but I think prefixing every class with DataTracker is a bit
ugly. My intention was for the package to fully identify their domain, but I
agree Transaction could perhaps be called LifecycleTransaction (or something)
to make it clearer).
We can certainly rollback Tracker to DataTracker, but since we now have a
lifecycle.View (no longer DataTracker.View), unless we also prefix that (or
move it back to an inner class - which I would be against: we have too many
monolithic classes in the codebase) I'm not sure it improves clarity, and I'm
personally in favour of reducing the number of characters where possible
(especially since "Data" is a bit vacuous in a database). I don't mind you
making the final call on the names since I don't have very strong feelings
about them, but wanted to put across my rationale.
Perhaps we can throw up a bikeshedding bat signal and get some other views?
(This sentence is meant to do that, if you're watching)
I've squashed all of the already reviewed commits. In the [2.2
branch|https://github.com/belliottsmith/cassandra/tree/8568-2.2] the new work
is separated into commits:
# addressing your clarification/nits concerns
# making a minor tweak to Transactional so that cleanup can be post- or pre-
abort; I realised that this was a potential mistake in the rebase onto that
functionality, since we want to ensure the compacting set always contains the
readers right up until the last moment.
# a trivial TODO, introducing a UniqueIdentifier for SSTableReader, that
permits our leak detection to kick in earlier around a lifecycle Transaction
# new tests
#* Since we're in a New World Order, I opted to introduce tests for every piece
of code I modified, not just the brand new functionality, so this may be a bit
of a slog to review, and I'm sorry about that :)
#* One of the old functionality tests spotted an inconsistency in the usage of
View.getOldestMemtable(), so I have removed this method and used a more
suitable mechanism in the two callers
#* There is a failing test covering old functionality that was too large to
sneak in along with the test coverage, and I will file a follow-up bug to
address it. It's not clear to me if it is at all serious or not, but it looks
like it might be.
> Impose new API on data tracker modifications that makes correct usage obvious
> and imposes safety
> ------------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-8568
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8568
> Project: Cassandra
> Issue Type: Bug
> Reporter: Benedict
> Assignee: Benedict
> Fix For: 2.2.0 rc1
>
>
> DataTracker has become a bit of a quagmire, and not at all obvious to
> interface with, with many subtly different modifiers. I suspect it is still
> subtly broken, especially around error recovery.
> I propose piggy-backing on CASSANDRA-7705 to offer RAII (and GC-enforced, for
> those situations where a try/finally block isn't possible) objects that have
> transactional behaviour, and with few simple declarative methods that can be
> composed simply to provide all of the functionality we currently need.
> See CASSANDRA-8399 for context
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
