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

jiraposter@reviews.apache.org
added a comment - 22/Apr/12 21:05
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 02/May/12 18:06
-----------------------------------------------------------
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

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

jiraposter@reviews.apache.org
added a comment - 05/May/12 16:45
-----------------------------------------------------------
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