> 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.
>
> Brock Noland wrote:
> Or as an alternative to the above you can catch Error, rollback and then
> re-throw...
>
> Arvind Prabhakar wrote:
> I feel that if the close() method never throws an exception, the idiom is
> perfectly fine in all cases. Besides, if an Error type does occur, then it is
> ok to leak tx resources. I do acknowledge that requiring all clients of this
> API to follow this idiom is a bit of a drag, but it ensures easy switching of
> the channel when necessary. It also gives an easy way to use
> telescoping/reference-counting semantics where necessary.
These two JUnit examples shows what I mean. Below a serious error is thrown:
@Test
public void testExample() throws Exception {
Event event = EventBuilder.withBody("test event".getBytes());
Channel channel = new MemoryChannel();
Context context = new Context();
Configurables.configure(channel, context);
Transaction tx = channel.getTransaction();
try {
tx.begin();
channel.put(event);
if(true) {
throw new Error("Error class means a serious problem occurred");
}
tx.commit();
} catch (Exception ex) {
tx.rollback();
throw ex;
} finally {
tx.close();
}
}
But all we get is:
java.lang.IllegalStateException: close() called when transaction is OPEN - you
must either commit or rollback first
at
com.google.common.base.Preconditions.checkState(Preconditions.java:172)
at
org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
at
org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
In order to handle this correctly we have to take additional action like so:
@Test
public void testExample() throws Exception {
Event event = EventBuilder.withBody("test event".getBytes());
Channel channel = new MemoryChannel();
Context context = new Context();
Configurables.configure(channel, context);
Transaction tx = channel.getTransaction();
try {
tx.begin();
channel.put(event);
if(true) {
throw new Error("Error class means a serious problem occurred");
}
tx.commit();
} catch (Exception ex) {
tx.rollback();
throw ex;
} catch (Error error) {
tx.rollback();
throw error;
} finally {
tx.close();
}
}
Now we get the real error:
java.lang.Error: Error class means a serious problem occurred
at
org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:57)
- 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
>
>
