----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4713/#review7380 -----------------------------------------------------------
Ship it! Thanks for the patch Brock, changes look good. Some minor feedback follows. Please rebase the patch when convenient and attach to the Jira. flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java <https://reviews.apache.org/r/4713/#comment16288> nit: this seems to be duplicating part of the functionality offered by the LifecycleState flag maintained by the abstract base class. It will be good to see if that can be reused here. flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java <https://reviews.apache.org/r/4713/#comment16284> nit:tab flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java <https://reviews.apache.org/r/4713/#comment16285> This call needs to follow the transaction idiom with the appropriate try/catch. Also, since this is initialization time code, it may be easier to do a bulk put in a single transaction. flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/WAL.java <https://reviews.apache.org/r/4713/#comment16291> Since these are getting accessed by worker thread and no longer propagated, they should be marked volatile? (or propagated). flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/DefaultLogicalNodeManager.java <https://reviews.apache.org/r/4713/#comment16283> The source runner interface is no longer used anywhere and should have been removed altogether (along with its subtypes). Since there is other cleanup in your patch, it would be great if you could remove that as well. - Arvind On 2012-04-13 20:12:25, Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4713/ > ----------------------------------------------------------- > > (Updated 2012-04-13 20:12:25) > > > Review request for Flume. > > > Summary > ------- > > Channel.start() is not being called. This is fixed in > DefaultLogicalNodeManager. > > Additionally RecoverableMemoryChannel now tracks it's own capacity due to the > MemoryChannel semantics being completely different. Basically, if we rely on > MemoryChannel capacity, then an error will be thrown when we commit the > MemoryChannelTransaction. However, we will have already committed this data > to disk. If we commit to MemoryChannel first (there by checking capacity) we > could fail to write to disk resulting in data which is only in memory. > > Also, I ran cleanup on modules touched. This removes whitespace, unused > imports, and adds @Override tags where needed. This is one time cleanup which > allows automated cleanup in the future. > > > This addresses bug FLUME-1121. > https://issues.apache.org/jira/browse/FLUME-1121 > > > Diffs > ----- > > flume-ng-core/src/test/java/org/apache/flume/channel/TestChannelUtils.java > 1421449 > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelConcurrency.java > 8dad0b2 > > flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannelTransaction.java > bc81f26 > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/FlumeConfiguration.java > d66f6d1 > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java > 1f0e8c6 > > flume-ng-core/src/test/java/org/apache/flume/channel/TestBasicChannelSemantics.java > 80020fc > > flume-ng-core/src/test/java/org/apache/flume/channel/AbstractBasicChannelSemanticsTest.java > 6e71e46 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java > 732cce5 > flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java ca5212e > flume-ng-core/src/main/java/org/apache/flume/sink/SinkType.java 6b08c09 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > b0485b1 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/WALDataFile.java > 9d4a1fd > > flume-ng-channels/flume-recoverable-memory-channel/src/test/java/org/apache/flume/channel/recoverable/memory/TestRecoverableMemoryChannel.java > edd8a8b > flume-ng-core/src/main/java/org/apache/flume/SinkProcessorType.java be1891b > flume-ng-core/src/main/java/org/apache/flume/conf/Configurables.java > 84492e5 > > flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java > 4722819 > flume-ng-core/src/main/java/org/apache/flume/lifecycle/LifecycleAware.java > f179de0 > flume-ng-core/src/main/java/org/apache/flume/sink/AbstractSink.java 2334059 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/WAL.java > 97ef796 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/SequenceIDBuffer.java > fa63b73 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java > 0622f27 > > flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/DefaultLogicalNodeManager.java > 07c3d0b > flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java > 225cd34 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java > 0b8a2c0 > > Diff: https://reviews.apache.org/r/4713/diff > > > Testing > ------- > > All unit tests pass and manual testing passes as well. > > > Thanks, > > Brock > >
