Flume
  1. Flume
  2. FLUME-1089

Transaction.close should be safe to call at any point

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: v1.2.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      We are struggling with error handling in regards to transactions. The general consensus is that it should be safe to call close on a transaction at any point.

      Discussion on flume-dev here:

      http://mail-archives.apache.org/mod_mbox/incubator-flume-dev/201203.mbox/%3CCAFukC=5X99U=aOZ5qMR_OoF=pZ9F7YHL6OFKyZu08UT4OR0wiw@mail.gmail.com%3E

      1. FLUME-1089-1.patch
        4 kB
        Brock Noland
      2. FLUME-1089-0.patch
        4 kB
        Brock Noland

        Activity

        Hide
        Brock Noland added a comment -

        Patch from RB

        Show
        Brock Noland added a comment - Patch from RB
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4655/ ----------------------------------------------------------- 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
        Hide
        Brock Noland added a comment -

        Patch from RB

        Show
        Brock Noland added a comment - Patch from RB
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        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.

        • Arvind

        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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4655/#review6810 ----------------------------------------------------------- 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. Arvind 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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.

        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?

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - 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. 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? 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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?

        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.

        • Arvind

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

        Show
        jiraposter@reviews.apache.org added a comment - 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? 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. Arvind ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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.

        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?

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - 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. 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? 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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?

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

        • Arvind

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

        Show
        jiraposter@reviews.apache.org added a comment - 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? 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). Arvind ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

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

        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

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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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). 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 ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

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

        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.

        • Arvind

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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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... 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. Arvind ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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)

        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?

        • Arvind

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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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) 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? Arvind ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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.

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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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. 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 } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. }

        Will McQueen wrote:

        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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) {

        throwable = t1; // for finally clause to know if exception was thrown

        Throwables.propagateIfInstanceOf(t1, Error.class);

        try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. }

        throw t1;

        } finally {

        if (!(throwable instanceof Error)) {

        try { bq. // txn.close(); bq. } catch (Throwable t) {

        if (throwable != null) { bq. t.initCause(throwable); bq. }

        throw t;

        }

        }

        }

        When I referred to OutOfMemoryException above, I meant OutOfMemoryError.

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. } Will McQueen wrote: 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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) { throwable = t1; // for finally clause to know if exception was thrown Throwables.propagateIfInstanceOf(t1, Error.class); try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. } throw t1; } finally { if (!(throwable instanceof Error)) { try { bq. // txn.close(); bq. } catch (Throwable t) { if (throwable != null) { bq. t.initCause(throwable); bq. } throw t; } } } When I referred to OutOfMemoryException above, I meant OutOfMemoryError. 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. }

        Will McQueen wrote:

        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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) {

        throwable = t1; // for finally clause to know if exception was thrown

        Throwables.propagateIfInstanceOf(t1, Error.class);

        try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. }

        throw t1;

        } finally {

        if (!(throwable instanceof Error)) {

        try { bq. // txn.close(); bq. } catch (Throwable t) {

        if (throwable != null) { bq. t.initCause(throwable); bq. }

        throw t;

        }

        }

        }

        Will McQueen wrote:

        When I referred to OutOfMemoryException above, I meant OutOfMemoryError.

        Catching Error and Throwable are all very dangerous things todo and should be done sparingly. It's too easy to hide serious errors. Working in OPS and DevOPS I have seen developers mess this up far too many times.

        I think we should look at JDBC, the most common transactional system, and improve upon that.

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. } Will McQueen wrote: 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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) { throwable = t1; // for finally clause to know if exception was thrown Throwables.propagateIfInstanceOf(t1, Error.class); try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. } throw t1; } finally { if (!(throwable instanceof Error)) { try { bq. // txn.close(); bq. } catch (Throwable t) { if (throwable != null) { bq. t.initCause(throwable); bq. } throw t; } } } Will McQueen wrote: When I referred to OutOfMemoryException above, I meant OutOfMemoryError. Catching Error and Throwable are all very dangerous things todo and should be done sparingly. It's too easy to hide serious errors. Working in OPS and DevOPS I have seen developers mess this up far too many times. I think we should look at JDBC, the most common transactional system, and improve upon that. 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * }

        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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally {

        * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * }

        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) { bq. throw new Error("Error class means a serious problem occurred"); bq. }
        bq. tx.commit();
        bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. }
        bq. }
        bq.
        bq. But all we get is:
        bq.
        bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first
        bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172)
        bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179)
        bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64)
        bq.
        bq. In order to handle this correctly we have to take additional action like so:
        bq.
        bq. @Test
        bq. public void testExample() throws Exception {
        bq. Event event = EventBuilder.withBody("test event".getBytes());
        bq. Channel channel = new MemoryChannel();
        bq. Context context = new Context();
        bq. Configurables.configure(channel, context);
        bq. Transaction tx = channel.getTransaction();
        bq. try {
        bq. tx.begin();
        bq. channel.put(event);
        bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. }

        tx.commit();

        } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. }

        }

        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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. }

        Will McQueen wrote:

        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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) {

        throwable = t1; // for finally clause to know if exception was thrown

        Throwables.propagateIfInstanceOf(t1, Error.class);

        try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. }

        throw t1;

        } finally {

        if (!(throwable instanceof Error)) {

        try { bq. // txn.close(); bq. } catch (Throwable t) {

        if (throwable != null) { bq. t.initCause(throwable); bq. }

        throw t;

        }

        }

        }

        Will McQueen wrote:

        When I referred to OutOfMemoryException above, I meant OutOfMemoryError.

        Brock Noland wrote:

        Catching Error and Throwable are all very dangerous things todo and should be done sparingly. It's too easy to hide serious errors. Working in OPS and DevOPS I have seen developers mess this up far too many times.

        I think we should look at JDBC, the most common transactional system, and improve upon that.

        I feel like we need to make a decision on what to do here. I found FLUME-1133 which is another place we are calling close and it's throwing an error which eats the real error. The current idiom seems to be quite prone to programmer error.

        • 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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * ... bq. * } finally { bq. * tx.close(); bq. * } 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 { bq. * tx.begin(); bq. * ... bq. * // ch.put(event) or ch.take() bq. * ... bq. * tx.commit(); bq. * readyForClose = true; bq. * } catch (Exception ex) { bq. * tx.rollback(); bq. * readyForClose = true; bq. * ... bq. * } finally { * if(readyForClose) { bq. * tx.close(); bq. * } else { bq. * tx.rollback(); bq. * tx.close(); bq. * } 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) { bq. throw new Error("Error class means a serious problem occurred"); bq. } bq. tx.commit(); bq. } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } finally { bq. tx.close(); bq. } bq. } bq. bq. But all we get is: bq. bq. java.lang.IllegalStateException: close() called when transaction is OPEN - you must either commit or rollback first bq. at com.google.common.base.Preconditions.checkState(Preconditions.java:172) bq. at org.apache.flume.channel.BasicTransactionSemantics.close(BasicTransactionSemantics.java:179) bq. at org.apache.flume.channel.TestMemoryChannel.testExample(TestMemoryChannel.java:64) bq. bq. In order to handle this correctly we have to take additional action like so: bq. bq. @Test bq. public void testExample() throws Exception { bq. Event event = EventBuilder.withBody("test event".getBytes()); bq. Channel channel = new MemoryChannel(); bq. Context context = new Context(); bq. Configurables.configure(channel, context); bq. Transaction tx = channel.getTransaction(); bq. try { bq. tx.begin(); bq. channel.put(event); bq. if(true) { bq. throw new Error("Error class means a serious problem occurred"); bq. } tx.commit(); } catch (Exception ex) { bq. tx.rollback(); bq. throw ex; bq. } catch (Error error) { bq. tx.rollback(); bq. throw error; bq. } finally { bq. tx.close(); bq. } } 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 { bq. //NEW bq. foo_1(); //No ISE thrown for "(NEW ---close()---> CLOSED)", and ISE thrown for "(NEW ---XXXrollback()XXX---> XXX)" bq. tx.begin(); bq. //OPEN bq. foo_2(); //ISE thrown for "(OPEN ---XXXclose()XXX---> XXX)", and ISE thrown for "(OPEN ---XXXrollback()XXX---> XXX)" bq. tx.commit(); bq. //COMPLETED bq. foo_3(); //No ISE thrown for "(COMPLETED ---close()---> CLOSED)", and ISE thrown for "(COMPLETED ---XXXrollback()XXX---> XXX)" bq. } catch (Exception ex) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Exception thrown bq. 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. bq. //COMPLETED bq. throw ex; bq. } catch (Error error) { bq. //NEW (due to foo_1), OPEN (due to foo_2), or COMPLETED (due to foo_3) when Error thrown bq. 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. bq. //COMPLETED (due to foo_2) bq. throw error; bq. } finally { bq. //NEW (due to foo_1), OPEN (due to foo_2), COMPLETED (due to foo_3) bq. tx.close(); //ISE thrown if got to this catch block due to foo_2 bq. //CLOSED bq. } Will McQueen wrote: 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 { bq. foo_1(); bq. // txn.begin(); bq. foo_2(); bq. // txn.commit(); bq. foo_3(); bq. } catch (Throwable t1) { throwable = t1; // for finally clause to know if exception was thrown Throwables.propagateIfInstanceOf(t1, Error.class); try { bq. // txn.rollback(); bq. } catch (Throwable t2) { bq. t2.initCause(t1); bq. throwable = t2; // for finally clause bq. throw t2; bq. } throw t1; } finally { if (!(throwable instanceof Error)) { try { bq. // txn.close(); bq. } catch (Throwable t) { if (throwable != null) { bq. t.initCause(throwable); bq. } throw t; } } } Will McQueen wrote: When I referred to OutOfMemoryException above, I meant OutOfMemoryError. Brock Noland wrote: Catching Error and Throwable are all very dangerous things todo and should be done sparingly. It's too easy to hide serious errors. Working in OPS and DevOPS I have seen developers mess this up far too many times. I think we should look at JDBC, the most common transactional system, and improve upon that. I feel like we need to make a decision on what to do here. I found FLUME-1133 which is another place we are calling close and it's throwing an error which eats the real error. The current idiom seems to be quite prone to programmer error. 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

          People

          • Assignee:
            Brock Noland
            Reporter:
            Brock Noland
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development