> 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?
>
>
> Will McQueen wrote:
> 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 McQueen wrote:
> Here's some code that shows the state of the txn at various places within
> the code. There are 4 states: NEW, OPEN, COMPLETED, CLOSED. "ISE" means
> "IllegalStateException", which can be thrown by rollback() or by close(). The
> methods foo_1(), foo_2(), and foo_3() are each assumed to throw a Throwable
> (or subclass of Throwable). The comments after each method show valid state
> transitions and invalid (with XXX) ones. This sample code with comments shows
> some cases that we may need to consider.
>
> try {
> //NEW
> foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE
> thrown for "(NEW ---XXXrollback()XXX---> XXX)"
> tx.begin();
> //OPEN
> foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE
> thrown for "(OPEN ---XXXrollback()XXX---> XXX)"
> tx.commit();
> //COMPLETED
> foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)",
> and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)"
> } catch (Exception ex) {
> //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to
> foo_3) when Exception thrown
> tx.rollback(); //ISE thrown if got to this catch block due to foo_1
> or foo_3 throwing Exception. The ISE will replace the Exception.
> //COMPLETED
> throw ex;
> } catch (Error error) {
> //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to
> foo_3) when Error thrown
> tx.rollback(); //ISE thrown if got to this catch block due to foo_1
> or foo_3 throwing Error. The ISE will replace the Error.
> //COMPLETED (due to foo_2)
> throw error;
> } finally {
> //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3)
> tx.close(); //ISE thrown if got to this catch block due to foo_2
> //CLOSED
> }
A possible solution is shown below. I could not think of a more elegant way to
do this, while still accounting for all cases. The goals I had in mind when
coding this were:
1) Propagate any Error without taking any additional action (eg, closing
resources, etc)
2) Use a causal chain of exceptions where needed, to traceback to original
Throwable.
Note that the Throwables class is from Guava.
Throwable throwable = null;
try {
foo_1();
// txn.begin();
foo_2();
// txn.commit();
foo_3();
} catch (Throwable t1) {
throwable = t1; // for finally clause to know if exception was
thrown
Throwables.propagateIfInstanceOf(t1, Error.class);
try {
// txn.rollback();
} catch (Throwable t2) {
t2.initCause(t1);
throwable = t2; // for finally clause
throw t2;
}
throw t1;
} finally {
if (!(throwable instanceof Error)) {
try {
// txn.close();
} catch (Throwable t) {
if (throwable != null) {
t.initCause(throwable);
}
throw t;
}
}
}
- 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
>
>
