cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benedict (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-10082) Transactional classes shouldn't also implement streams, channels, etc
Date Tue, 18 Aug 2015 08:51:46 GMT


Benedict commented on CASSANDRA-10082:

bq. as it seems a confusingly arbitrary method call for changing the semantics.

Your comments don't address this.

The method you've introduced has the following problems with it:

# It is called getOutputStream, and SequentialWriter is already an OutputStream
# It gives you an OS with different semantics, but backed by the same SW, _both of which exist
at the same time_
#* This is especially implied as OK, given the "get" semantics, as though it were a property,
despite its effectively overriding the transactionality of the SW
# In no way does it let the user know there has been a change in semantics

My point of making a "general purpose" utility was to avoid all of this, by creating a wrapper
class that clearly modifies the semantics of a Tranasctional OutputStream, returning you a
non-transactional one. It could live in SW if you prefer, but it should make it very clear
what is happening, and should _take as its argument_ the SW to make it very clear the SW no
longer exists as a separate entity, but has been encapsulated. It's not a major modification.

That all said, I would also really much rather we decoupled the sstable-specific behaviour
of SW from those we might employ elsewhere, and avoid this problem further up the hierarchy.
The whole shebang needs a refactor. This is fine as a stop gap, but it's all considerably
uglier than it should be.

> Transactional classes shouldn't also implement streams, channels, etc
> ---------------------------------------------------------------------
>                 Key: CASSANDRA-10082
>                 URL:
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Blake Eggleston
>            Assignee: Blake Eggleston
>         Attachments: 0001-replacing-SequentialWriter-OutputStream-extension-wi.patch
> Since the close method on the Transactional interface means "abort if commit hasn't been
called", mixing Transactional and AutoCloseable interfaces where close means "we're done here"
is pretty much never the right thing to do. 
> The only class that does this is SequentialWriter. It's not used in a way that causes
a problem, but it's still a potential hazard for future development.
> The attached patch replaces the SequentialWriter OutputStream implementation with a wrapper
class that implements the expected behavior on close, and adds a warning to the Transactional
interface. It also adds a unit test that demonstrates the problem without the fix.

This message was sent by Atlassian JIRA

View raw message