----------------------------------------------------------- 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 > >
