Review Board 1.7.22


FLUME-1516. Write dual checkpoints.

Review Request #8899 - Created Jan. 9, 2013 and submitted

Hari Shreedharan
FLUME-1516
Reviewers
Flume
flume-git
Added support for backup and retrieval of checkpoint.
Added new unit tests. Current tests pass.
Total:
56
Open:
36
Resolved:
20
Dropped:
0
Status:
From:
Description From Last Updated Status
This is duplicated elsewhere Brock Noland March 14, 2013, 5:18 p.m. Open
Should we warn if it fails? Brock Noland March 14, 2013, 5:18 p.m. Open
listFiles can return null Brock Noland March 14, 2013, 5:18 p.m. Open
this is the same as backupFile, why are creating it? What if it fails? Brock Noland March 14, 2013, 5:18 p.m. Open
I think that "Backup" would be a more consistent name than "BackUp" Brock Noland March 14, 2013, 5:18 p.m. Open
listFiles can return null. Brock Noland March 14, 2013, 5:18 p.m. Open
Did the backup fail or is it still in progress? Brock Noland March 14, 2013, 5:18 p.m. Open
We should catch Throwable here incase a runtime exception or Error is thrown. Brock Noland March 14, 2013, 5:18 p.m. Open
We should log the exception here Brock Noland March 14, 2013, 5:18 p.m. Open
This will be set to null by default, no need to have = null. Brock Noland March 14, 2013, 5:18 p.m. Open
What if they specify the same value twice? That would be ugly. Also since this property is not plural perhaps ... Brock Noland March 14, 2013, 5:18 p.m. Open
If useDualCheckpoints is true we should warn that we cannot? We what if people put none, or three checkpoint directories? ... Brock Noland March 14, 2013, 5:18 p.m. Open
We don't do reconfiguration any longer, should we remove the reconfig stuff? Brock Noland March 14, 2013, 5:18 p.m. Open
I know it's not your change, but can you change this to info? Brock Noland March 14, 2013, 5:18 p.m. Open
This can return null, we should handle that. Brock Noland March 14, 2013, 5:18 p.m. Open
Yes, Will do. Hari Shreedharan March 14, 2013, 6:10 p.m. Open
Will update in next patch with a !null if condition Hari Shreedharan March 14, 2013, 6:10 p.m. Open
The reason there is a backupComplete file is to ensure that we know that all files were copied. The idea ... Hari Shreedharan March 14, 2013, 6:10 p.m. Open
will fix Hari Shreedharan March 14, 2013, 6:10 p.m. Open
will add check Hari Shreedharan March 14, 2013, 6:10 p.m. Open
The previous backup is still happening, so we skip this checkpoint, rather than trying to abort the backup, cleanup the ... Hari Shreedharan March 14, 2013, 6:10 p.m. Open
Agreed Hari Shreedharan March 14, 2013, 6:10 p.m. Open
will do Hari Shreedharan March 14, 2013, 6:10 p.m. Open
This was done to get the IDE to shut up ;) Hari Shreedharan March 14, 2013, 6:10 p.m. Open
Yeah, this was just me being lazy :P Hari Shreedharan March 14, 2013, 6:10 p.m. Open
yeah, I think we should Hari Shreedharan March 14, 2013, 6:10 p.m. Open
might make sense to do so. Hari Shreedharan March 14, 2013, 6:10 p.m. Open
yeah, this one confuses a lot of people! Hari Shreedharan March 14, 2013, 6:10 p.m. Open
will do Hari Shreedharan March 14, 2013, 6:10 p.m. Open
Unless there is a reason to use getCanonicalPath, which throws an IOException, might as well just use the toString(). Brock Noland March 25, 2013, 9:51 p.m. Open
If listFiles() returns null, we say later the checkpoint was successful when it was not. I think we should throw ... Brock Noland March 25, 2013, 9:51 p.m. Open
What buffer size is used for this copy? Brock Noland March 25, 2013, 9:51 p.m. Open
Same as backupExists? Brock Noland March 25, 2013, 9:51 p.m. Open
What kind of buffer size are we using during this copy? Brock Noland March 25, 2013, 9:51 p.m. Open
As opposed to this, how about using drainPermits(). Then if we ever return > 1, throw an illegal state exception. ... Brock Noland March 25, 2013, 9:51 p.m. Open
This is a misconfiguration. I think we should throw an exception as opposed to ignoring this. Brock Noland March 25, 2013, 9:51 p.m. Open
Review request changed
Updated (April 5, 2013, 6:45 p.m.)
  • changed from pending to submitted