You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Arvind Prabhakar <ar...@apache.org> on 2012/05/02 19:05:30 UTC
Re: Review Request: FLUME-1131: ChannelProcessor does not handle
transactions appropriately
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-1131: ChannelProcessor does not handle
transactions appropriately
Posted by Brock Noland <br...@cloudera.com>.
> 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 {
> > 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.
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
>
>