[
https://issues.apache.org/jira/browse/FLUME-935?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13201342#comment-13201342
]
[email protected] commented on FLUME-935:
-----------------------------------------------------
bq. On 2012-02-06 08:40:29, Juhani Connolly wrote:
bq. > I'm not seeing any unit tests attached to this?
bq. > Personally I still feel that handling all checked exceptions other than
InterruptedException should be the channels responsibility, and it should be
their responsibility to decide whether to propagate them as ChannelExceptions
or do something else.
bq. > Regarding the counting, I personally found it to make semantics awkward
and unclear(what do we do if we get two begins and one commits then the other
rollbacks!?) so it's loss isn't a big deal to me. In fact I might even go so
far as saying that I think a transaction should only be openable once, as they
are now entirely threadlocal.
bq. > These are just my personal opinions though and I think the patch should
be good to go, and perhaps it can be further polished in a different issue.
I think what motivated me to allow Exception in the first place is the fact
that ChannelException is itself unchecked, and so we're forcing everyone to
"hide" any checked exceptions themselves. In my experience, that means that
many developers will simply swallow them (either wholesale or by simply logging
them) instead of allowing them to propagate, and therefore I tend to build
frameworks that create controlled spaces in which checked exceptions may be
thrown freely, allowing the framework to handle/wrap/log the exceptions
appropriately. It also has the added benefit of keeping _generic_ try..catch
blocks out of implementation code, reducing implementation code volume and
slighly increasing its legibility. It's not a perfect solution, but it's
worked well for me.
However, in this context I think I'm realizing that the fact that
ChannelException is itself unchecked makes it easy for developers to wrap
exceptions instead of swallowing them, and that that combined with the review
process is likely to prevent such "swallowing" problems from occurring.
InterruptedException, as you have pointed out, is a special case in that it
must be propagated specially.
If were were to change the Channel and Transaction interfaces to throw
InterruptedException, then the code in this patch could be further simplified
as they could then allow InterruptedException to propagate naturally like any
other exception. I didn't want to do that in this patch, however, as it would
likely require changes to any existing clients of those interfaces throughout
the codebase. Maybe I'll open a separate JIRA for that, pointing out the
simplification to be made to this code if/when it happens.
Meanwhile, I'll remove the throwing of IOException as you suggest and upload
yet another version of this patch (r5) shortly.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/#review4829
-----------------------------------------------------------
On 2012-02-06 14:48:58, Peter Newcomb wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3516/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-06 14:48:58)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Implementation of FLUME-935 as new classes BasicChannelSemantics,
BasicTransactionSemantics, and ChannelUtils. It might be better to fold
BasicChannelSemantics into AbstractChannel and rename BasicTransactionSemantics
to AbstractTransaction, but doing that would require refactoring of existing
classes that extend AbstractChannel.
bq.
bq.
bq. This addresses bug FLUME-935.
bq. https://issues.apache.org/jira/browse/FLUME-935
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java
PRE-CREATION
bq.
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java
PRE-CREATION
bq.
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/ChannelException.java
1240900
bq.
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicChannelSemantics.java
PRE-CREATION
bq.
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSemantics.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/3516/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. I am using these in production code, and they have survived significant
integration testing there, including failure modes. Note also that these
classes are largely error handling and precondition testing code designed to
test the correctness of the code around them.
bq.
bq. A fairly comprehensive set of unit tests around BasicChannelSemantics and
BasicTransactionSemantics is included.
bq.
bq.
bq. Thanks,
bq.
bq. Peter
bq.
bq.
> Create abstract implementations of basic channel/transaction semantics
> ----------------------------------------------------------------------
>
> Key: FLUME-935
> URL: https://issues.apache.org/jira/browse/FLUME-935
> Project: Flume
> Issue Type: Improvement
> Components: Channel
> Affects Versions: v1.0.0
> Reporter: Peter Newcomb
> Assignee: Peter Newcomb
> Priority: Minor
> Fix For: v1.1.0
>
>
> Correctly executing or checking the state transitions for channels and
> transactions is nontrivial. It would be helpful to have a correct
> implementation of each that can be used either directly or as a reference
> when creating new channels or clients of channels.
> Specifically, on the client side it would be nice to package the try {
> begin() ... commit() } catch { rollback() } finally { close() } code, with
> all the appropriate exception propagation and logging code, so that it need
> not be repeated constantly.
> On the channel side, it'd be nice to have a packaged implementation of the
> implied ThreadLocal semantics of the Transaction class, along with
> Preconditions checking to make sure that clients follow the try { begin() ...
> commit() } catch { rollback() } finally { close() } pattern.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
