You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Peter Newcomb <pe...@walmartlabs.com> on 2012/02/06 07:37:40 UTC

Re: Review Request: Implementation of FLUME-935

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/
-----------------------------------------------------------

(Updated 2012-02-06 06:37:40.844119)


Review request for Flume.


Changes
-------

This patch incorporates suggested changes and fixes, an additional fix or two, and fairly comprehensive unit tests, though not fully reusable ones as I earlier suggested might be nice to have.

One fix had to do with handling Errors as well as Exceptions--I ran across the need for this in practice, when a codec was not installed properly.  This fix required modifying ChannelException to accept a Throwable as the cause, not just an Exception.  I hope that's OK.

With regard to the reference counting question regarding the support for nested transactions: I was doing the reference counting there in order to limit the nesting behavior to between begin() and commit()/rollback(), so that one would not necessarily have to call begin() [or getTransaction()] and close() in perfect nested pairs--I was considering close() a resource release, not a fundamental part of the transaction process.

However, as I thought about nested transactions, I realized that they seem to be a non-requirement.  I thought that they were at least a nice-to-have based on the JDBC channel implementation, but nothing else seems to expect them.  I therefore removed them entirely pending identification of need and/or further work and discussion to figure out how they should work.


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

  /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 
  /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/3516/diff


Testing (updated)
-------

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

Re: Review Request: Implementation of FLUME-935

Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/#review5045
-----------------------------------------------------------

Ship it!


+1. Changes look good to me. Please attach the patch to the JIRA.

- Arvind


On 2012-02-06 23:09:15, Peter Newcomb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3516/
> -----------------------------------------------------------
> 
> (Updated 2012-02-06 23:09:15)
> 
> 
> 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/AbstractBasicChannelSemanticsTest.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 1241246 
>   /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 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java PRE-CREATION 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelUtils.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 is included.
> 
> 
> Thanks,
> 
> Peter
> 
>

Re: Review Request: Implementation of FLUME-935

Posted by Juhani Connolly <ju...@gmail.com>.

> On 2012-02-07 00:45:14, Juhani Connolly wrote:
> > Everything looks good to me, except your ChannelException change broke one of the jdbc tests:
> > 
> > TestJdbcChannelProvider
> > org.apache.flume.channel.jdbc.TestJdbcChannelProvider
> > testDerbyChannelCapacity(org.apache.flume.channel.jdbc.TestJdbcChannelProvider)
> > java.lang.NoSuchMethodError: org.apache.flume.ChannelException.<init>(Ljava/lang/String;Ljava/lang/Exception;)V
> > 	at org.apache.flume.channel.jdbc.JdbcChannelException.<init>(JdbcChannelException.java:29)
> > 	at org.apache.flume.channel.jdbc.impl.JdbcChannelProviderImpl.persistEvent(JdbcChannelProviderImpl.java:245)
> > 	at org.apache.flume.channel.jdbc.TestJdbcChannelProvider.testDerbyChannelCapacity(TestJdbcChannelProvider.java:113)
> > 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> > 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
> > 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> > 	at java.lang.reflect.Method.invoke(Method.java:616)
> > 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
> > 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
> > 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
> > 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
> > 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
> > 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
> > 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
> > 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
> > 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
> > 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
> > 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
> > 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
> > 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
> > 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
> > 	at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
> > 	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
> > 	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
> > 	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
> > 	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
> > 	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
> > 	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
> > 
> > Apart from that I think it looks good, hopefully we'll hear from one of the committers.

Hmm, I couldn't figure out what was wrong with the code and after another clean/test it doesn't happen anymore so scratch that


- Juhani


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/#review4847
-----------------------------------------------------------


On 2012-02-06 23:09:15, Peter Newcomb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3516/
> -----------------------------------------------------------
> 
> (Updated 2012-02-06 23:09:15)
> 
> 
> 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/AbstractBasicChannelSemanticsTest.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 1241246 
>   /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 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java PRE-CREATION 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelUtils.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 is included.
> 
> 
> Thanks,
> 
> Peter
> 
>

Re: Review Request: Implementation of FLUME-935

Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/#review4847
-----------------------------------------------------------


Everything looks good to me, except your ChannelException change broke one of the jdbc tests:

TestJdbcChannelProvider
org.apache.flume.channel.jdbc.TestJdbcChannelProvider
testDerbyChannelCapacity(org.apache.flume.channel.jdbc.TestJdbcChannelProvider)
java.lang.NoSuchMethodError: org.apache.flume.ChannelException.<init>(Ljava/lang/String;Ljava/lang/Exception;)V
	at org.apache.flume.channel.jdbc.JdbcChannelException.<init>(JdbcChannelException.java:29)
	at org.apache.flume.channel.jdbc.impl.JdbcChannelProviderImpl.persistEvent(JdbcChannelProviderImpl.java:245)
	at org.apache.flume.channel.jdbc.TestJdbcChannelProvider.testDerbyChannelCapacity(TestJdbcChannelProvider.java:113)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:616)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

Apart from that I think it looks good, hopefully we'll hear from one of the committers.

- Juhani


On 2012-02-06 23:09:15, Peter Newcomb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3516/
> -----------------------------------------------------------
> 
> (Updated 2012-02-06 23:09:15)
> 
> 
> 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/AbstractBasicChannelSemanticsTest.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 1241246 
>   /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 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java PRE-CREATION 
>   /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelUtils.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 is included.
> 
> 
> Thanks,
> 
> Peter
> 
>

Re: Review Request: Implementation of FLUME-935

Posted by Peter Newcomb <pe...@walmartlabs.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/
-----------------------------------------------------------

(Updated 2012-02-06 23:09:15.089801)


Review request for Flume.


Changes
-------

Removed allowance for and specific handling of IOException, added ChannelUtils unit tests based on utilities abstracted out from TestBasicChannelSemantics, simplified a few things, and fixed some documentation text.


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

  /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/AbstractBasicChannelSemanticsTest.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 1241246 
  /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 
  /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java PRE-CREATION 
  /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelUtils.java PRE-CREATION 

Diff: https://reviews.apache.org/r/3516/diff


Testing (updated)
-------

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 is included.


Thanks,

Peter

Re: Review Request: Implementation of FLUME-935

Posted by Peter Newcomb <pe...@walmartlabs.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/
-----------------------------------------------------------

(Updated 2012-02-06 14:48:58.249584)


Review request for Flume.


Changes
-------

Oops, this version includes the unit tests and changes to ChannelException.  I'm still learning how to use post-review.


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

  /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

Re: Review Request: Implementation of FLUME-935

Posted by Peter Newcomb <pe...@walmartlabs.com>.

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

Re: Review Request: Implementation of FLUME-935

Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3516/#review4829
-----------------------------------------------------------


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.

- Juhani


On 2012-02-06 06:37:40, Peter Newcomb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3516/
> -----------------------------------------------------------
> 
> (Updated 2012-02-06 06:37:40)
> 
> 
> 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/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 
>   /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/channel/ChannelUtils.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
> 
>