[
https://issues.apache.org/jira/browse/FLUME-762?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100224#comment-13100224
]
[email protected] commented on FLUME-762:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1739/#review1815
-----------------------------------------------------------
Prasad, no worry about the spacing, I'm guilty of this quite a bit.
My main concern is
1) all three test essentially look the same -- just from reading the tests I
don't know what the difference is supposed to be.
2) the tests don't seem to verify anything, and their descriptions make me
think an exception would be expected (but they aren't).
also have some nits and a few other suggestions.
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4166>
LOG instead of println.
flume-core/src/main/java/com/cloudera/flume/core/connector/DirectDriver.java
<https://reviews.apache.org/r/1739/#comment4167>
Log when interrupted?
flume-core/src/main/java/com/cloudera/flume/core/connector/DirectDriver.java
<https://reviews.apache.org/r/1739/#comment4168>
Log when interrupted?
flume-core/src/main/java/com/cloudera/flume/core/connector/DirectDriver.java
<https://reviews.apache.org/r/1739/#comment4176>
nit: Are these tabs?
flume-core/src/main/java/com/cloudera/flume/core/connector/DirectDriver.java
<https://reviews.apache.org/r/1739/#comment4169>
I think this is what I mean in my previous comment:
LOG.warn("Exception in sink: " + sink.getName(), eI);
The warn/info/error method's signature is essentially:
void warn(String string, Exception ex)
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4172>
Nit: you could just just have a Exception as a field, and pass it in as a
constructor argument. When it is time to throw it, just throw it.
I think it would be cleaner that way. Bonus: This would make the tests
easier to read as well! (oh, this one throws IOException, and this one throws
InterruptedException)!
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4170>
LOG.info
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4171>
LOG.info
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4174>
How can I tell this did what was expected?
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4178>
The description makes me think this should be
@Test(expected=IOException.class)
(it isn't and shouldn't be).
Please checksomething to convince the reader it works.
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4173>
How can I tell if this did what i expected?
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4177>
Description makes me think this should be:
@Test(expected=InterruptedException.class)
Needs better comment or better verfication of something in test.
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
<https://reviews.apache.org/r/1739/#comment4175>
How can I tell this did what was expected?
- jmhsieh
On 2011-09-08 07:25:11, Prasad Mujumdar wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1739/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-09-08 07:25:11)
bq.
bq.
bq. Review request for jmhsieh.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. If the source or sink throws an exception, close, reopen and retry it.
This way the flow can continue after minor/recoverable errors.
bq.
bq.
bq. This addresses bug FLUME-762.
bq. https://issues.apache.org/jira/browse/FLUME-762
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
flume-core/src/main/java/com/cloudera/flume/core/connector/DirectDriver.java
0466394
bq.
flume-core/src/test/java/com/cloudera/flume/core/connector/TestDirectDriverExp.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/1739/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Prasad
bq.
bq.
> DirectDriver should reopen the source/sink if the append or next throws an
> exception
> ------------------------------------------------------------------------------------
>
> Key: FLUME-762
> URL: https://issues.apache.org/jira/browse/FLUME-762
> Project: Flume
> Issue Type: Improvement
> Components: Node
> Reporter: Prasad Mujumdar
> Assignee: Prasad Mujumdar
> Attachments: flume-762.patch
>
>
> Currently, if source or sink throws an exception, the driver bails out
> resulting the everything to shut down. Alternately, we can try to close and
> reopen the source or sink, and then attempt to continue. This way we can
> avoid blocking the flow. If the reopen fails, then it can bail out.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
