> On 2012-02-06 08:40:29, Juhani Connolly wrote: > > I'm not seeing any unit tests attached to this? > > 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. > > 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. > > 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3516/ > ----------------------------------------------------------- > > (Updated 2012-02-06 14:48:58) > > > Review request for Flume. > > > Summary > ------- > > 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. > > > This addresses bug FLUME-935. > https://issues.apache.org/jira/browse/FLUME-935 > > > Diffs > ----- > > > /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java > PRE-CREATION > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java > PRE-CREATION > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/ChannelException.java > 1240900 > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicChannelSemantics.java > PRE-CREATION > > /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSemantics.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3516/diff > > > Testing > ------- > > 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. > > A fairly comprehensive set of unit tests around BasicChannelSemantics and > BasicTransactionSemantics is included. > > > Thanks, > > Peter > >
