> 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.
>
> Brock Noland wrote:
> 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)
>
> Arvind Prabhakar wrote:
> My apologies for dragging this out so far, but I do see your point. One
> way to address both these concerns is to catch a Throwable instead. Do you
> think that would work?
>
I agree with what Arvind mentioned earlier about Error being thrown, "if an
Error type does occur, then it is ok to leak tx resources". According to
JavaDocs for Error, "An Error is a subclass of Throwable that indicates serious
problems that a reasonable application should not try to catch". My
understanding is that the JVM can cause an Error (or one of its subclasses) to
be thrown within any or all of your threads (effectively inserting a "throw
<Error>" into any thread, and at any time... and so after any bytecode
instruction). Not only that, but I believe that the JVM can throw an Error
multiple times (eg, OutOfMemoryException). So when we encounter an Error, I
feel we should just propagate it without taking any additional action.
- Will
-----------------------------------------------------------
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
>
>
