> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > 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?
I'd like to keep those, since we can avoid seeks. I have seen situations where
there have been like 400 files, and just reading the entire files will still
take a very long time. The changes related to the seek are still relatively
small and there is a unit test to keep track of this. If we don't keep those,
we will read all files fully - I think we should avoid that.
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStore.java,
> > line 49
> > <https://reviews.apache.org/r/8899/diff/7/?file=270820#file270820line49>
> >
> > The changes to this class seem like they belong in
> > EventQueueBackingStoreFile.
Yep. Will move it.
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java,
> > line 159
> > <https://reviews.apache.org/r/8899/diff/7/?file=270822#file270822line159>
> >
> > This file create could fail. We should log if it does.
Sure. Will add this
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java,
> > line 253
> > <https://reviews.apache.org/r/8899/diff/7/?file=270822#file270822line253>
> >
> > Since this a Throwable and not an Exception I think the variable should
> > be throwable as opposed to ex.
Will do.
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java,
> > line 131
> > <https://reviews.apache.org/r/8899/diff/7/?file=270824#file270824line131>
> >
> > 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?
Will do.
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java,
> > line 1044
> > <https://reviews.apache.org/r/8899/diff/7/?file=270827#file270827line1044>
> >
> > I think "filesToDelete" would be better named "pendingDeletes" or
> > something like that.
Will do.
> On March 21, 2013, 3:01 a.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java,
> > line 1068
> > <https://reviews.apache.org/r/8899/diff/7/?file=270827#file270827line1068>
> >
> > Why not have a single code path? It will change the current behavior
> > slightly but I don't see a huge issue with that.
Do you mean that we can hold on to the files till the next checkpoint even if
dual checkpoints is disabled? That makes sense. I will do that in the next patch
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18196
-----------------------------------------------------------
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
>
>
