> On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFactory.java, > > line 75 > > <https://reviews.apache.org/r/6680/diff/9/?file=149829#file149829line75> > > > > s/clode/close
fixed > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, > > line 42 > > <https://reviews.apache.org/r/6680/diff/9/?file=149830#file149830line42> > > > > Nit: Both implementations of this class should be final, since we are > > calling non-final methods in the constructor. (in the V3 class). fixed > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, > > line 46 > > <https://reviews.apache.org/r/6680/diff/9/?file=149830#file149830line46> > > > > 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? v1 of the patch removes the header from the file. After thinking about it for a long time, I decided that was a risk operation. Several times it corrupted it (I am sure it was a bug on my part) so I decided the simplest thing was to leave the header in tact. It will waste a small amount of space but only like 8K. > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFile.java, > > line 119 > > <https://reviews.apache.org/r/6680/diff/9/?file=149830#file149830line119> > > > > Nit: overwriteMap.isEmpty() fixed > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java, > > lines 47-65 > > <https://reviews.apache.org/r/6680/diff/9/?file=149832#file149832line47> > > > > 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. Fixed, I just call the get* methods to populate the protos with the defaults. > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java, > > line 85 > > <https://reviews.apache.org/r/6680/diff/9/?file=149832#file149832line85> > > > > logFileIDReferenceCounts.containsKey(logFileID) should not return true > > at this point right? fixed > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java, > > lines 149-152 > > <https://reviews.apache.org/r/6680/diff/9/?file=149832#file149832line149> > > > > 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. As mentioned above, we are leaving a small amount of data there, but it's the simplest and lowest risk way of moving forward. > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/EventQueueBackingStoreFileV3.java, > > lines 177-179 > > <https://reviews.apache.org/r/6680/diff/9/?file=149832#file149832line177> > > > > 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. I'd prefer to just leave the data. We know it's irrelevant because we have .meta file and we also update the version in the checkpoint file to 3. > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java, > > lines 365-373 > > <https://reviews.apache.org/r/6680/diff/9/?file=149834#file149834line365> > > > > 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. sure, fixed > On Sept. 7, 2012, 12:56 a.m., Hari Shreedharan wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java, > > line 690 > > <https://reviews.apache.org/r/6680/diff/9/?file=149835#file149835line690> > > > > 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) fixed - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6680/#review10819 ----------------------------------------------------------- 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 > >
