Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume >> mail # dev >> Review Request: FLUME-1516. Write dual checkpoints.


Copy link to this message
-
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: