> On 2012-04-30 19:22:02, Arvind Prabhakar wrote: > > Thanks for the patch Brock, changes look good. Some minor feedback follows. > > > > Please rebase the patch when convenient and attach to the Jira.
Thank you for your review! Attached to the JIRA! > On 2012-04-30 19:22:02, Arvind Prabhakar wrote: > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java, > > line 123 > > <https://reviews.apache.org/r/4713/diff/3/?file=101674#file101674line123> > > > > nit:tab done > On 2012-04-30 19:22:02, Arvind Prabhakar wrote: > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java, > > line 82 > > <https://reviews.apache.org/r/4713/diff/3/?file=101674#file101674line82> > > > > 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. I agree, I have created FLUME-1169 to implement this checking in the superclass. > On 2012-04-30 19:22:02, Arvind Prabhakar wrote: > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java, > > lines 144-148 > > <https://reviews.apache.org/r/4713/diff/3/?file=101674#file101674line144> > > > > 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. Done > On 2012-04-30 19:22:02, Arvind Prabhakar wrote: > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/WAL.java, > > lines 92-93 > > <https://reviews.apache.org/r/4713/diff/3/?file=101676#file101676line92> > > > > Since these are getting accessed by worker thread and no longer > > propagated, they should be marked volatile? (or propagated). I figured eventual consistency for this parameters was fine, however I have marked them volatile. - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4713/#review7380 ----------------------------------------------------------- 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 > >
