You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@flume.apache.org by Brock Noland <br...@cloudera.com> on 2012/12/07 19:15:55 UTC

Review Request: FLUME-1761: FileChannel can NPE when log metadata file is empty

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

Review request for Flume.

Description
-------

1) Adds better error messages if for any reason Commit,Take,Put,Rollback encountered an null pointer. Shouldn't happen.
2) Fast replay handles the case where we have an empty log or metadata file.
3) LogFileFactory throws an informative error message when the meta file is empty but the log not.
4) LogFileFactory throws an EOF when log file and meta are empty sot he log can be ignored during replay.

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

Diffs
-----

flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/CheckpointRebuilder.java 748f49a
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Commit.java 62f4451
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileFactory.java 4783448
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV3.java b4c197e
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java d47b1c8
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Rollback.java cc9ce86
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Take.java e61bf7e
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/TransactionEventRecord.java 70098a0
flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java a165d6a

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

Testing
-------

Added tests which test 2, 3, 4. The behavior is the same for 1 we just have a better error message.

Thanks,

Brock Noland

Re: Review Request: FLUME-1761: FileChannel can NPE when log metadata file is empty

Posted by Brock Noland <br...@cloudera.com>.


> On Dec. 11, 2012, 6:29 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java, lines 287-288
> > <https://reviews.apache.org/r/8414/diff/2/?file=235796#file235796line287>
> >
> >     The log.replay() call here does not replay anything since there is nothing in the logs at this point. I think we need to pass a param to the doTestReplayFailsWithAllEmptyLogMetaData() method, and based on that call setUseFastReplay(true) on the builder (and delete the checkpoint files) - otherwise this test is not really using fast replay - is it?

Good catch!


- Brock


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


On Dec. 11, 2012, 4:38 p.m., Brock Noland wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8414/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2012, 4:38 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Description
> -------
> 
> 1) Adds better error messages if for any reason Commit,Take,Put,Rollback encountered an null pointer. Shouldn't happen.
> 2) Fast replay handles the case where we have an empty log or metadata file.
> 3) LogFileFactory throws an informative error message when the meta file is empty but the log not.
> 4) LogFileFactory throws an EOF when log file and meta are empty sot he log can be ignored during replay.
> 
> 
> This addresses bug FLUME-1761.
>     https://issues.apache.org/jira/browse/FLUME-1761
> 
> 
> Diffs
> -----
> 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/CheckpointRebuilder.java 748f49a 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Commit.java 62f4451 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileFactory.java 4783448 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV3.java b4c197e 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java d47b1c8 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Rollback.java cc9ce86 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Take.java e61bf7e 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/TransactionEventRecord.java 70098a0 
>   flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java a165d6a 
> 
> Diff: https://reviews.apache.org/r/8414/diff/
> 
> 
> Testing
> -------
> 
> Added tests which test 2, 3, 4. The behavior is the same for 1 we just have a better error message.
> 
> 
> Thanks,
> 
> Brock Noland
> 
>

Re: Review Request: FLUME-1761: FileChannel can NPE when log metadata file is empty

Posted by Hari Shreedharan <hs...@cloudera.com>.

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


Generally looks good. I think one test needs some change as noted below.


flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java
<https://reviews.apache.org/r/8414/#comment30436>

    The log.replay() call here does not replay anything since there is nothing in the logs at this point. I think we need to pass a param to the doTestReplayFailsWithAllEmptyLogMetaData() method, and based on that call setUseFastReplay(true) on the builder (and delete the checkpoint files) - otherwise this test is not really using fast replay - is it?


- Hari Shreedharan


On Dec. 7, 2012, 6:15 p.m., Brock Noland wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8414/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2012, 6:15 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Description
> -------
> 
> 1) Adds better error messages if for any reason Commit,Take,Put,Rollback encountered an null pointer. Shouldn't happen.
> 2) Fast replay handles the case where we have an empty log or metadata file.
> 3) LogFileFactory throws an informative error message when the meta file is empty but the log not.
> 4) LogFileFactory throws an EOF when log file and meta are empty sot he log can be ignored during replay.
> 
> 
> This addresses bug FLUME-1761.
>     https://issues.apache.org/jira/browse/FLUME-1761
> 
> 
> Diffs
> -----
> 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/CheckpointRebuilder.java 748f49a 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Commit.java 62f4451 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileFactory.java 4783448 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV3.java b4c197e 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java d47b1c8 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Rollback.java cc9ce86 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Take.java e61bf7e 
>   flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/TransactionEventRecord.java 70098a0 
>   flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java a165d6a 
> 
> Diff: https://reviews.apache.org/r/8414/diff/
> 
> 
> Testing
> -------
> 
> Added tests which test 2, 3, 4. The behavior is the same for 1 we just have a better error message.
> 
> 
> Thanks,
> 
> Brock Noland
> 
>

Re: Review Request: FLUME-1761: FileChannel can NPE when log metadata file is empty

Posted by Brock Noland <br...@cloudera.com>.

(Updated Dec. 11, 2012, 4:38 p.m.)

Review request for Flume.

Changes
-------

Fixed test

Description
-------

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

Diffs (updated)
-----

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

Testing
-------

Added tests which test 2, 3, 4. The behavior is the same for 1 we just have a better error message.

Thanks,

Brock Noland