Flume
  1. Flume
  2. FLUME-1131

ChannelProcessor does not handle transactions appropriately

    Details

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

      Description

      If an Exception is thrown in ChannelProcessor.processEvent whilst getting an exception, tx will be null and when rollback is called on tx, an NPE will be thrown.

      12/04/18 19:03:17 ERROR source.PollableSourceRunner: Unhandled exception, logging and sleeping for 5000ms
      java.lang.NullPointerException
      	at org.apache.flume.channel.ChannelProcessor.processEvent(ChannelProcessor.java:183)
      	at org.apache.flume.source.SequenceGeneratorSource.process(SequenceGeneratorSource.java:48)
      	at org.apache.flume.source.PollableSourceRunner$PollingRunner.run(PollableSourceRunner.java:131)
      	at java.lang.Thread.run(Thread.java:662)
      
      1. FLUME-1131-0.patch
        10 kB
        Brock Noland
      2. FLUME-1131-1.patch
        10 kB
        Brock Noland

        Issue Links

          Activity

          Hide
          Brock Noland added a comment -

          This problem is actually all over ChannelProcessor.

          Show
          Brock Noland added a comment - This problem is actually all over ChannelProcessor.
          Hide
          Brock Noland added a comment -

          one patch to rule them all

          Show
          Brock Noland added a comment - one patch to rule them all
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for Flume.

          Summary
          -------

          ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown.

          This addresses bug FLUME-1131.
          https://issues.apache.org/jira/browse/FLUME-1131

          Diffs


          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION
          flume-ng-core/pom.xml b9f1e12
          flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e

          Diff: https://reviews.apache.org/r/4837/diff

          Testing
          -------

          Tests added and 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/4837/ ----------------------------------------------------------- Review request for Flume. Summary ------- ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown. This addresses bug FLUME-1131 . https://issues.apache.org/jira/browse/FLUME-1131 Diffs flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION flume-ng-core/pom.xml b9f1e12 flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e Diff: https://reviews.apache.org/r/4837/diff Testing ------- Tests added and 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/4837/#review7474
          -----------------------------------------------------------

          Thanks for the patch Brock. Changes look good. I do have my tx-nit request below for you to consider. Also noted some other feedback.

          flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java
          <https://reviews.apache.org/r/4837/#comment16519>

          Perhaps this is equivalent to:

          ...
          } catch (Throwable th) {
          if (tx != null) {
          try

          { tx.rollback(); }

          catch (Exception ex)

          { LOG.warn("exception...", ex); }

          }
          Throwables.propagate(th);
          } finally {
          if (tx != null)

          { tx.close(); }

          }

          If you agree, I would request that we follow this logic instead in other places as well below.

          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java
          <https://reviews.apache.org/r/4837/#comment16521>

          Missing the license header.

          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java
          <https://reviews.apache.org/r/4837/#comment16520>

          Thanks for adding these tests.

          • Arvind

          On 2012-04-22 20:04:13, Brock Noland wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4837/

          -----------------------------------------------------------

          (Updated 2012-04-22 20:04:13)

          Review request for Flume.

          Summary

          -------

          ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown.

          This addresses bug FLUME-1131.

          https://issues.apache.org/jira/browse/FLUME-1131

          Diffs

          -----

          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION

          flume-ng-core/pom.xml b9f1e12

          flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e

          Diff: https://reviews.apache.org/r/4837/diff

          Testing

          -------

          Tests added and 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/4837/#review7474 ----------------------------------------------------------- Thanks for the patch Brock. Changes look good. I do have my tx-nit request below for you to consider. Also noted some other feedback. flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java < https://reviews.apache.org/r/4837/#comment16519 > Perhaps this is equivalent to: ... } catch (Throwable th) { if (tx != null) { try { tx.rollback(); } catch (Exception ex) { LOG.warn("exception...", ex); } } Throwables.propagate(th); } finally { if (tx != null) { tx.close(); } } If you agree, I would request that we follow this logic instead in other places as well below. flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java < https://reviews.apache.org/r/4837/#comment16521 > Missing the license header. flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java < https://reviews.apache.org/r/4837/#comment16520 > Thanks for adding these tests. Arvind On 2012-04-22 20:04:13, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4837/ ----------------------------------------------------------- (Updated 2012-04-22 20:04:13) Review request for Flume. Summary ------- ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown. This addresses bug FLUME-1131 . https://issues.apache.org/jira/browse/FLUME-1131 Diffs ----- flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION flume-ng-core/pom.xml b9f1e12 flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e Diff: https://reviews.apache.org/r/4837/diff Testing ------- Tests added and tests pass. Thanks, Brock
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-05-02 17:05:30, Arvind Prabhakar wrote:

          > flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java, line 1

          > <https://reviews.apache.org/r/4837/diff/2/?file=103887#file103887line1>

          >

          > Missing the license header.

          fixed

          On 2012-05-02 17:05:30, Arvind Prabhakar wrote:

          > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java, lines 121-145

          > <https://reviews.apache.org/r/4837/diff/2/?file=103886#file103886line121>

          >

          > Perhaps this is equivalent to:

          >

          > ...

          > } catch (Throwable th) {

          > if (tx != null) {

          > try { bq. > tx.rollback(); bq. > } catch (Exception ex) { bq. > LOG.warn("exception...", ex); bq. > }

          > }

          > Throwables.propagate(th);

          > } finally {

          > if (tx != null) { bq. > tx.close(); bq. > }

          > }

          >

          > If you agree, I would request that we follow this logic instead in other places as well below.

          Agreed, updated in the latest patch.

          • Brock

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

          On 2012-05-05 15:44:22, Brock Noland wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4837/

          -----------------------------------------------------------

          (Updated 2012-05-05 15:44:22)

          Review request for Flume.

          Summary

          -------

          ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown.

          This addresses bug FLUME-1131.

          https://issues.apache.org/jira/browse/FLUME-1131

          Diffs

          -----

          flume-ng-core/pom.xml b798b34

          flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e

          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION

          Diff: https://reviews.apache.org/r/4837/diff

          Testing

          -------

          Tests added and tests pass.

          Thanks,

          Brock

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-05-02 17:05:30, Arvind Prabhakar wrote: > flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java, line 1 > < https://reviews.apache.org/r/4837/diff/2/?file=103887#file103887line1 > > > Missing the license header. fixed On 2012-05-02 17:05:30, Arvind Prabhakar wrote: > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java, lines 121-145 > < https://reviews.apache.org/r/4837/diff/2/?file=103886#file103886line121 > > > Perhaps this is equivalent to: > > ... > } catch (Throwable th) { > if (tx != null) { > try { bq. > tx.rollback(); bq. > } catch (Exception ex) { bq. > LOG.warn("exception...", ex); bq. > } > } > Throwables.propagate(th); > } finally { > if (tx != null) { bq. > tx.close(); bq. > } > } > > If you agree, I would request that we follow this logic instead in other places as well below. Agreed, updated in the latest patch. Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4837/#review7474 ----------------------------------------------------------- On 2012-05-05 15:44:22, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4837/ ----------------------------------------------------------- (Updated 2012-05-05 15:44:22) Review request for Flume. Summary ------- ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown. This addresses bug FLUME-1131 . https://issues.apache.org/jira/browse/FLUME-1131 Diffs ----- flume-ng-core/pom.xml b798b34 flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION Diff: https://reviews.apache.org/r/4837/diff Testing ------- Tests added and tests pass. Thanks, Brock
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2012-05-05 15:44:22.206077)

          Review request for Flume.

          Changes
          -------

          Updated patch which addresses the review items.

          Summary
          -------

          ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown.

          This addresses bug FLUME-1131.
          https://issues.apache.org/jira/browse/FLUME-1131

          Diffs (updated)


          flume-ng-core/pom.xml b798b34
          flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e
          flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION

          Diff: https://reviews.apache.org/r/4837/diff

          Testing
          -------

          Tests added and 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/4837/ ----------------------------------------------------------- (Updated 2012-05-05 15:44:22.206077) Review request for Flume. Changes ------- Updated patch which addresses the review items. Summary ------- ChannelProcessor did not handle transactions well, in some cases if anything but ChannelException was thrown, the close in the finally would throw an exception as rollback was not called. In other cases if a subclass of error was thrown the same would occurred. Additionally, if getTransaction threw an exception the null transaction value was not handled and an NPE would be thrown. This addresses bug FLUME-1131 . https://issues.apache.org/jira/browse/FLUME-1131 Diffs (updated) flume-ng-core/pom.xml b798b34 flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java eb6460e flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelProcessor.java PRE-CREATION Diff: https://reviews.apache.org/r/4837/diff Testing ------- Tests added and tests pass. Thanks, Brock
          Hide
          Brock Noland added a comment -

          Patch from RB.

          Show
          Brock Noland added a comment - Patch from RB.

            People

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

              Dates

              • Created:
                Updated:

                Development