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 2013/04/04 20:20:39 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/#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>.
> 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
>
>