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/#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: