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