> On 2012-04-10 00:01:17, Arvind Prabhakar wrote:
> > Thanks for the patch Brock. I think what this patch does is forces a state
> > transition on close no matter what. This has the potential of covering up
> > for programmatic problems that could lead to resource/tx leaks in the
> > system which I feel should not happen. If a component is buggy, the other
> > components around it should not try to coverup.
> >
> > Another way to look at it is - the close() method should not throw an
> > exception ever. This can be further reinforced by having a thread local
> > transaction that is discarded on close.
>
> Brock Noland wrote:
> I can agree with that.
>
> The new code would do the state transition (which means a new transaction
> is gotten on getTransaction()) and then call doClose(). Correct?
>
> Arvind Prabhakar wrote:
> My view on it is that there are two parts to this problem:
>
> 1. If someone calls close() when the tx is not in the correct state, that
> should fail with an exception. This signals a bad/buggy implementation that
> should be identified aggressively and fixed.
>
> 2. If someone calls close() when the tx is in the correct state, that
> should never fail. This will ensure that good code is not penalized for
> implementation issues of the tx provider.
>
>
>
> Brock Noland wrote:
> In my understanding from the email chain "Channel/Transaction States" was
> that like a DB statement, you should be able to call close() should be safe
> to call at any point in time. If work is uncommitted that work is thrown
> away.
>
> If we require rollback or commit to be called before close, then every
> source/sink needs to catch Throwable, call rollback and rethrow so that close
> can be called in the finally block. Thoughts?
>
> Arvind Prabhakar wrote:
> The use of transaction must be done in an idiomatic manner as described
> in it's api:
>
> * Channel ch = ...
> * Transaction tx = ch.getTransaction();
> * try {
> * tx.begin();
> * ...
> * // ch.put(event) or ch.take()
> * ...
> * tx.commit();
> * } catch (Exception ex) {
> * tx.rollback();
> * ...
> * } finally {
> * tx.close();
> * }
>
> If the caller is using this idiom, then it is a guarantee that the state
> transition will occur correctly, and that for every begin there is a close.
> As you can see from this idiom, the close should not be throwing an exception
> (and implicitly the begin too).
>
> Brock Noland wrote:
> The issue with the idom above is that if anything is thrown which not an
> Exception (e.g. subclass of Error), an exception will be thrown in the
> finally clause and that more serious problem will be eaten. The only way this
> can been handled is:
>
> * boolean readyForClose = false;
> * Channel ch = ...
> * Transaction tx = ch.getTransaction();
> * try {
> * tx.begin();
> * ...
> * // ch.put(event) or ch.take()
> * ...
> * tx.commit();
> * readyForClose = true;
> * } catch (Exception ex) {
> * tx.rollback();
> * readyForClose = true;
> * ...
> * } finally {
> * if(readyForClose) {
> * tx.close();
> * } else {
> * tx.rollback();
> * tx.close();
> * }
>
> It seems quite a lot of effort to push on our users and is quite bug
> prone.
Or as an alternative to the above you can catch Error, rollback and then
re-throw...
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4655/#review6810
-----------------------------------------------------------
On 2012-04-05 03:05:51, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4655/
> -----------------------------------------------------------
>
> (Updated 2012-04-05 03:05:51)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Allowing the calling of transaction.close() at any point of time.
>
>
> This addresses bug FLUME-1089.
> https://issues.apache.org/jira/browse/FLUME-1089
>
>
> Diffs
> -----
>
>
> flume-ng-core/src/main/java/org/apache/flume/channel/BasicTransactionSemantics.java
> 403cbca
>
> flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java
> 80020fc
>
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java
> bc81f26
>
> Diff: https://reviews.apache.org/r/4655/diff
>
>
> Testing
> -------
>
> Unit tests pass.
>
>
> Thanks,
>
> Brock
>
>
