-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18196
-----------------------------------------------------------
Looks great Hari!
One item I am not convinced we need is the
backupCheckpoint{writeorderid/position}. It adds complexity for not a terrible
amount of benefit. In the case we cannot seek into the file we will have to
read all the logs. However, we won't have to process the logs in terms of the
queue which is the big bottleneck. What do you think about removing those
fields to make this simpler?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStore.java
<https://reviews.apache.org/r/8899/#comment38385>
The changes to this class seem like they belong in
EventQueueBackingStoreFile.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38383>
This file create could fail. We should log if it does.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38381>
Since this a Throwable and not an Exception I think the variable should be
throwable as opposed to ex.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment38384>
As opposed to allowing this to be null and complicating the check below,
how about passing in "" as a default and calling trim() on the returned value?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java
<https://reviews.apache.org/r/8899/#comment38386>
I think "filesToDelete" would be better named "pendingDeletes" or something
like that.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java
<https://reviews.apache.org/r/8899/#comment38387>
Why not have a single code path? It will change the current behavior
slightly but I don't see a huge issue with that.
- Brock Noland
On March 14, 2013, 11:43 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 14, 2013, 11:43 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added support for backup and retrieval of checkpoint.
>
>
> This addresses bug FLUME-1516.
> https://issues.apache.org/jira/browse/FLUME-1516
>
>
> Diffs
> -----
>
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStore.java
> b136eb0
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFactory.java
> a19bdb4
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
> 4115505
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java
> 451a9d4
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
> ff42d19
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannelConfiguration.java
> 24368b3
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java
> 1ed9547
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java
> 6ffc824
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java
> 1db3717
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV3.java
> f51935c
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/ReplayHandler.java
> fa4fd9d
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
> 7094d3c
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/proto/ProtosFactory.java
> e6d4957
> flume-ng-channels/flume-file-channel/src/main/proto/filechannel.proto
> 3a4e828
>
> flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannelBase.java
> 3da09ab
>
> flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannelRestart.java
> 170dc72
>
> flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestUtils.java
> ba653e6
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
