You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Hari Shreedharan <hs...@cloudera.com> on 2013/03/04 21:44:21 UTC
Re: Review Request: FLUME-1516. Write dual checkpoints.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated March 4, 2013, 8:44 p.m.)
Review request for Flume.
Changes
-------
Rebased on trunk.
Description
-------
Added support for backup and retrieval of checkpoint.
This addresses bug FLUME-1516.
https://issues.apache.org/jira/browse/FLUME-1516
Diffs (updated)
-----
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 d98209b
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 0f9456b
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java 7da8c49
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
Diff: https://reviews.apache.org/r/8899/diff/
Testing
-------
Added new unit tests. Current tests pass.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review17876
-----------------------------------------------------------
Hari, looks good and thanks for the hard work!! Below is a first pass.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37844>
This is duplicated elsewhere
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37843>
Should we warn if it fails?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37841>
listFiles can return null
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37842>
this is the same as backupFile, why are creating it? What if it fails?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37850>
I think that "Backup" would be a more consistent name than "BackUp"
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37851>
listFiles can return null.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37845>
Did the backup fail or is it still in progress?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37847>
We should catch Throwable here incase a runtime exception or Error is thrown.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37846>
We should log the exception here
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37854>
This will be set to null by default, no need to have = null.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37855>
What if they specify the same value twice? That would be ugly. Also since this property is not plural perhaps would we should introduce a new property, BACKPOINT_BACKUP_DIR?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37853>
If useDualCheckpoints is true we should warn that we cannot? We what if people put none, or three checkpoint directories? I think we should use Splliter.on(",").trimResults().omitEmptyStrings() and then do a little more sanity checking on this.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37852>
We don't do reconfiguration any longer, should we remove the reconfig stuff?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java
<https://reviews.apache.org/r/8899/#comment37848>
I know it's not your change, but can you change this to info?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
<https://reviews.apache.org/r/8899/#comment37849>
This can return null, we should handle that.
- Brock Noland
On March 14, 2013, 7:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 14, 2013, 7:31 a.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
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
> 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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
> On March 25, 2013, 9:51 p.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 184
> > <https://reviews.apache.org/r/8899/diff/8/?file=274457#file274457line184>
> >
> > What kind of buffer size are we using during this copy?
>
> Hari Shreedharan wrote:
> Looks like Guava uses a buffer size of 4K.
I think a buffer size like 64KB will make this faster.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18364
-----------------------------------------------------------
On March 25, 2013, 9:03 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 25, 2013, 9:03 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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On March 25, 2013, 9:51 p.m., Brock Noland wrote:
> > Another round of items. Have you done any manual testing of this patch?
I did some basic testing by simply deleting the checkpoint.
> On March 25, 2013, 9:51 p.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 156
> > <https://reviews.apache.org/r/8899/diff/8/?file=274457#file274457line156>
> >
> > What buffer size is used for this copy?
Looks like Guava uses a buffer size of 4K.
> On March 25, 2013, 9:51 p.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 153
> > <https://reviews.apache.org/r/8899/diff/8/?file=274457#file274457line153>
> >
> > If listFiles() returns null, we say later the checkpoint was successful when it was not. I think we should throw an exception.
Makes sense. Will do.
> On March 25, 2013, 9:51 p.m., Brock Noland wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 184
> > <https://reviews.apache.org/r/8899/diff/8/?file=274457#file274457line184>
> >
> > What kind of buffer size are we using during this copy?
Looks like Guava uses a buffer size of 4K.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18364
-----------------------------------------------------------
On March 25, 2013, 9:03 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 25, 2013, 9:03 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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18364
-----------------------------------------------------------
Another round of items. Have you done any manual testing of this patch?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38570>
Unless there is a reason to use getCanonicalPath, which throws an IOException, might as well just use the toString().
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38565>
If listFiles() returns null, we say later the checkpoint was successful when it was not. I think we should throw an exception.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38566>
What buffer size is used for this copy?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38564>
Same as backupExists?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38563>
What kind of buffer size are we using during this copy?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment38568>
As opposed to this, how about using drainPermits(). Then if we ever return > 1, throw an illegal state exception. This ensures we are using the semaphore correctly.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment38562>
This is a misconfiguration. I think we should throw an exception as opposed to ignoring this.
- Brock Noland
On March 25, 2013, 9:03 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 25, 2013, 9:03 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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On April 5, 2013, 2:35 p.m., Brock Noland wrote:
> > Hari, great work!! Can you attach the patch to the JIRA so I can run tests and if they pass, commit?
Done.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18700
-----------------------------------------------------------
On April 5, 2013, 12:16 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated April 5, 2013, 12:16 a.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
> flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18700
-----------------------------------------------------------
Hari, great work!! Can you attach the patch to the JIRA so I can run tests and if they pass, commit?
- Brock Noland
On April 5, 2013, 12:16 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated April 5, 2013, 12:16 a.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
> flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18718
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On April 5, 2013, 12:16 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated April 5, 2013, 12:16 a.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
> flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated April 5, 2013, 12:16 a.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 (updated)
-----
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
flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
Diff: https://reviews.apache.org/r/8899/diff/
Testing
-------
Added new unit tests. Current tests pass.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated April 4, 2013, 9:32 p.m.)
Review request for Flume.
Changes
-------
Incorporate feedback from Brock
Description
-------
Added support for backup and retrieval of checkpoint.
This addresses bug FLUME-1516.
https://issues.apache.org/jira/browse/FLUME-1516
Diffs (updated)
-----
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
flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
Diff: https://reviews.apache.org/r/8899/diff/
Testing
-------
Added new unit tests. Current tests pass.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On April 4, 2013, 6:20 p.m., Brock Noland wrote:
> > Hari,
> >
> > Looks great! We don't have a test which tests a slow backup process? If not, perhaps we should have a boolean member variable called
> >
> > testingSlowBackup
> >
> > which is set in a test and causes a sleep. There is a nice/easy reflection libary we have used elsewhere to set this if needed.
> >
> >
Done
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18685
-----------------------------------------------------------
On April 4, 2013, 9:32 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated April 4, 2013, 9:32 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
> flume-ng-doc/sphinx/FlumeUserGuide.rst 16fba45
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review18685
-----------------------------------------------------------
Hari,
Looks great! We don't have a test which tests a slow backup process? If not, perhaps we should have a boolean member variable called
testingSlowBackup
which is set in a test and causes a sleep. There is a nice/easy reflection libary we have used elsewhere to set this if needed.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39082>
Let's add the name of the fc to this thread so we can differentiate it amongst file channels.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39091>
Let's add an assertion such that and another that backupCompletedSem.drainPermits() returns 0 inside this method.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39080>
If we cannot delete the backup file, I think we should bail. If we cannot do that, how can we write files to do the backup?
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39081>
Similar to above, if the backup file does in fact exist, something is wrong since it was deleted above.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39083>
I think if(backupFiles == null) {
return false;
}
is more clear. Right now if have this trailing else with a return statement sitting down there.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment39084>
Let's move the if check to the caller. It's more clear to the reader.
Let's add an assertion on checkpointBackUpExecutor being non-null with a better error message than the default since this object can be null.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment39085>
I am not sure I agree with this. Here we have someone who specifically set useDualCheckpoints to true. Maybe they didn't realize that they needed to set a backup directory. Now we are going to just ignore that setting all the while they think they have dual checkpoints enabled...
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannelConfiguration.java
<https://reviews.apache.org/r/8899/#comment39086>
I think we kind of informally agreed new configuration parameters should be camel caps? Here we are adding one camel caps and one - delimited.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java
<https://reviews.apache.org/r/8899/#comment39087>
Let's just do an assertion here that backupCheckpointPosition is also greater than 0.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
<https://reviews.apache.org/r/8899/#comment39088>
I think this if condition should be reverse. Since we are acting on both conditions no reason to use != in the if clause.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
<https://reviews.apache.org/r/8899/#comment39090>
Should we do a fsync to make sure that the data has been written to disk? We are very careful to ensure data is consistent on disk for the normal checkpoints, I think that makes sense here as well.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
<https://reviews.apache.org/r/8899/#comment39089>
If we should never reach here, then we should throw an error if we do reach there.
flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannelRestart.java
<https://reviews.apache.org/r/8899/#comment39092>
Why not just have
overrides.put(FileChannelConfiguration.USE_DUAL_CHECKPOINTS, String.valueOf(backup));
as opposed to create a new method for one line of code?
flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannelRestart.java
<https://reviews.apache.org/r/8899/#comment39093>
Similarly this method can be replaced with
Assert.assertTrue(!backup || backupRestored);
- Brock Noland
On March 26, 2013, 10:52 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 26, 2013, 10:52 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
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated March 26, 2013, 10:52 p.m.)
Review request for Flume.
Changes
-------
File copy changes. Fixed one bug where a restored checkpoint/pre-existing checkpoint will lead to backup being disabled.
Description
-------
Added support for backup and retrieval of checkpoint.
This addresses bug FLUME-1516.
https://issues.apache.org/jira/browse/FLUME-1516
Diffs (updated)
-----
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
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated March 25, 2013, 9:03 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 (updated)
-----
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
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Incorporate Brock's feedback. Fixed one bug where the backup checkpoint position was not being updated in the SequentialReader.
Description
-------
Added support for backup and retrieval of checkpoint.
This addresses bug FLUME-1516.
https://issues.apache.org/jira/browse/FLUME-1516
Diffs (updated)
-----
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
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Brock Noland <br...@cloudera.com>.
> On March 14, 2013, 6:10 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 154
> > <https://reviews.apache.org/r/8899/diff/6/?file=270479#file270479line154>
> >
> > The reason there is a backupComplete file is to ensure that we know that all files were copied. The idea is that if we were in the middle of copying (only some files were copied, not all), then this indicates the copying was incomplete. This would be for the case where the checkpoint is somehow corrupt and we try to retrieve the backup - at that point, we need to be sure the backup actually contains all files required (this makes it extensible, in case we add more files to the checkpoint)
That is fine, I just meant the same file object exists a few lines up and we should handle the case where the create fails.
> On March 14, 2013, 6:10 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, line 185
> > <https://reviews.apache.org/r/8899/diff/6/?file=270479#file270479line185>
> >
> > The previous backup is still happening, so we skip this checkpoint, rather than trying to abort the backup, cleanup the previous one and start the checkpoint. (If the checkpoint interval is too low, then there may never be a backup if we do that)
Ok great, then we should clarify this in the msg.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review17888
-----------------------------------------------------------
On March 14, 2013, 7:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 14, 2013, 7:31 a.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
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/#review17888
-----------------------------------------------------------
Thanks for the review, Brock! I will submit an updated patch soon. Thanks!
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37865>
Yes, Will do.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37866>
Will update in next patch with a !null if condition
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37867>
The reason there is a backupComplete file is to ensure that we know that all files were copied. The idea is that if we were in the middle of copying (only some files were copied, not all), then this indicates the copying was incomplete. This would be for the case where the checkpoint is somehow corrupt and we try to retrieve the backup - at that point, we need to be sure the backup actually contains all files required (this makes it extensible, in case we add more files to the checkpoint)
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37869>
will fix
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37871>
will add check
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37872>
The previous backup is still happening, so we skip this checkpoint, rather than trying to abort the backup, cleanup the previous one and start the checkpoint. (If the checkpoint interval is too low, then there may never be a backup if we do that)
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37873>
Agreed
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java
<https://reviews.apache.org/r/8899/#comment37874>
will do
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37875>
This was done to get the IDE to shut up ;)
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37876>
Yeah, this was just me being lazy :P
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37877>
yeah, I think we should
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java
<https://reviews.apache.org/r/8899/#comment37878>
might make sense to do so.
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java
<https://reviews.apache.org/r/8899/#comment37879>
yeah, this one confuses a lot of people!
flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java
<https://reviews.apache.org/r/8899/#comment37880>
will do
- Hari Shreedharan
On March 14, 2013, 7:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8899/
> -----------------------------------------------------------
>
> (Updated March 14, 2013, 7:31 a.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
>
> Diff: https://reviews.apache.org/r/8899/diff/
>
>
> Testing
> -------
>
> Added new unit tests. Current tests pass.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1516. Write dual checkpoints.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8899/
-----------------------------------------------------------
(Updated March 14, 2013, 7:31 a.m.)
Review request for Flume.
Changes
-------
Rebased on trunk.
Description
-------
Added support for backup and retrieval of checkpoint.
This addresses bug FLUME-1516.
https://issues.apache.org/jira/browse/FLUME-1516
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/8899/diff/
Testing
-------
Added new unit tests. Current tests pass.
Thanks,
Hari Shreedharan