----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6680/#review10819 -----------------------------------------------------------
Awesome patch, Brock! This was absolutely required. Thanks a lot for putting so much time and effort into this. This looks almost good to go, I have a few relatively minor comments. Several places where new builders are created inside loops. All builders have a clear method. Rather than allocating a new build each time in the loop, we should reuse the one builder - if only one/few properties change we should set only those, or call clear and set everything we need. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFactory.java <https://reviews.apache.org/r/6680/#comment23756> s/clode/close flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java <https://reviews.apache.org/r/6680/#comment23757> Nit: Both implementations of this class should be final, since we are calling non-final methods in the constructor. (in the V3 class). flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java <https://reviews.apache.org/r/6680/#comment23758> This should not be in the base EventQueueBackingStore class. For the V2 class, the header size is 1029, but for V3 it is really much smaller, right? Then why hardcode this size? Otherwise we have old stuff in a checkpoint file which got upgraded, and this isn't a great idea(even if we never read them again). I realize this would make it expensive to truncate, how about 0 out all unused headers in the upgrade method? flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java <https://reviews.apache.org/r/6680/#comment23861> Nit: overwriteMap.isEmpty() flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java <https://reviews.apache.org/r/6680/#comment23759> Looks like if the metadata file is created - the file is first written to, then read from. This can easily be optimized to read from the file only if the checkpoint is not new. Also, if there is no metadata file - does calling getVersion, getHead and getSize make sense? Even in case an upgrade is done, if the metadata file is gone, I don't understand how calling these methods would work. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java <https://reviews.apache.org/r/6680/#comment23863> logFileIDReferenceCounts.containsKey(logFileID) should not return true at this point right? flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java <https://reviews.apache.org/r/6680/#comment23761> In Checkpoint V2, we have a bunch of headers which were moved to the metadata file in V3. When we upgrade from V2 to V3, we should remove these headers and update the queueHead to reflect that(or at least zero them out so we don't have outdated data in the checkpoint file), since the same data is available in the metadata file. It looks like only the version is being updated here. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java <https://reviews.apache.org/r/6680/#comment23864> How about 0-ing out all the legacy headers, so we don't need to move stuff around the entire file, but we can still not have outdated headers in the file. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java <https://reviews.apache.org/r/6680/#comment23762> Why this change? This creates an unnecessary copy of the entire set. This set is already created just for returning from this method, so it does not matter if the calling code changes it. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java <https://reviews.apache.org/r/6680/#comment23763> queue.getFileIDs() returns a copy of the active file IDs set. It is safe to change. This change creates an extra copy of the file IDs set(in addition to the set which is created) flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileFactory.java <https://reviews.apache.org/r/6680/#comment23868> Shouldn't this be called logFile? flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV2.java <https://reviews.apache.org/r/6680/#comment23869> LogFileV2.class flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java <https://reviews.apache.org/r/6680/#comment23764> Example of new builder created for each loop iteration. - Hari Shreedharan On Sept. 5, 2012, 7:57 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6680/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2012, 7:57 p.m.) > > > Review request for Flume. > > > Description > ------- > > Very sorry for the enormous patch. I don't see a way around a large patch due > to the nature of this JIRA. This is a first crack at a patch. I will be > reviewing it again and possibly providing an updated patch without any > reviews. However, I wanted to get feedback early. > > This change creates file format versions 3, which allows field updates such > as FLUME-1485 to be done without changing the format version and as such > impact users. > > This new format is entirely packed by Protocol Buffers with the exception of > the memory mapped file we use as the queue. This last file has a checkpoint > in progress marker the remaining portion is a memory mapped long buffer. > > All data metadata was moved to seperate files so it can be updated without > random writes. > > Where as before, given a checkpoint directory and log directory we would have: > > chkpt/checkpoint > log/log-1 > log/log-2 > > Where as now we have: > > chkpt/checkpoint > chkpt/checkpoint.meta > log/log-1 > log/log-1.meta > log/log-2 > log/log-2.meta > > The .meta files contain all metadata and the files no longer contain a > VERSION prefix. The .meta files do contain the a version identifer. > > This patch is backwards compatible in that when it starts up: > > -Old checkpoint is upgraded > -Only new v3 log files are written > -Old v2 log files are still readable > > This process is tested during unit tests to the regression tests added in > FLUME-1431. The long term plan would be to remove the V2 code after a couple > releases. > > To support this funcationality, the following were changed: > > -IO was removed from FlumeEventQueue and operates on an abstract class > EventQueueBackingStore which has a contrete implementation for V2 and V3. > -Checkpoint/Log file readers/writes are not obtained from factories which > choose the correct object or in the case of the event queue execute the > upgrade process > > > This addresses bug FLUME-1487. > https://issues.apache.org/jira/browse/FLUME-1487 > > > Diffs > ----- > > flume-ng-channels/flume-file-channel/pom.xml 7556f9f > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/CheckpointRebuilder.java > 32b5324 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Commit.java > 2c92d28 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStore.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFactory.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV2.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventPointer.java > e40cd8c > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java > 8085d22 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java > e13ecc4 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java > 2867fc7 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileFactory.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV2.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFileV3.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogUtils.java > 09956a4 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java > c3fda6b > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/ReplayHandler.java > 4e908ec > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Rollback.java > 6e4e1fc > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Serialization.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Take.java > cb575f9 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/TransactionEventRecord.java > 67c68f8 > flume-ng-channels/flume-file-channel/src/main/proto/filechannel.proto > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestCheckpoint.java > 2893538 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestEventQueueBackingStoreFactory.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannel.java > 35521d1 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFlumeEventQueue.java > d09ddde > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java > e923a30 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLogFile.java > 193cd2b > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLogRecord.java > 9f6adc7 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestTransactionEventRecord.java > c73b11b > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestTransactionEventRecordV2.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestTransactionEventRecordV3.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestUtils.java > e64f856 > pom.xml 52a842d > > Diff: https://reviews.apache.org/r/6680/diff/ > > > Testing > ------- > > Unit tests pass. I will be running some manual tests as well and will update > when I do so. > > > Thanks, > > Brock Noland > >
