----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5349/#review8322 -----------------------------------------------------------
Generally looks good. I have posted some comments inline, mostly related to removing synchronization from the methods. Most methods are synchronized currently to make sure other threads see the changes that these threads make, or this thread sees the changes the other threads make. Also in a couple of places, there is some minor formatting issue. trunk/flume-ng-core/src/main/java/org/apache/flume/CounterGroup.java <https://reviews.apache.org/r/5349/#comment17941> Extra whitespaces. trunk/flume-ng-core/src/main/java/org/apache/flume/CounterGroup.java <https://reviews.apache.org/r/5349/#comment17942> Extra whitespaces. trunk/flume-ng-core/src/main/java/org/apache/flume/CounterGroup.java <https://reviews.apache.org/r/5349/#comment17935> Shouldn't this be synchronized? "counters" is essentially a shared variable that can be updated by various threads. So if this is not synchronized a thread calling toString is not guaranteed to see all updates. The updates we are talking about are inserts of new key-value pairs. Since each value in the map is an atomic long, updates from one thread will be seen by another. I'd prefer to see the counters set to volatile. Again the same is true for the name too, but it is much less of an issue since the name can be set through the constructor, so I'd like to leave the set/getName synchronized. trunk/flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java <https://reviews.apache.org/r/5349/#comment17937> Removing synchronization of this affects the happens-before relationship. This is because the name would be set by the conf-poller thread when the agent starts up. So other threads calling getName() may not see the name which was set. These are synchronized getters and setters, there is not much overhead - we should probably leave them as is. trunk/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java <https://reviews.apache.org/r/5349/#comment17940> Indentation should have 2 spaces here, instead of 4. trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/5349/#comment17939> Indentation should have 2 spaces here, instead of 4 trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/5349/#comment17938> Indentation should have 2 spaces here, instead of 4. - Hari Shreedharan On June 17, 2012, 6:19 a.m., Mubarak Seyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5349/ > ----------------------------------------------------------- > > (Updated June 17, 2012, 6:19 a.m.) > > > Review request for Flume. > > > Description > ------- > > Logging output says name:null after initiating agent shutdown. Source and > Sink will call setName() of CounterGroup. > > > This addresses bug FLUME-1161. > https://issues.apache.org/jira/browse/FLUME-1161 > > > Diffs > ----- > > trunk/flume-ng-core/src/main/java/org/apache/flume/CounterGroup.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/AbstractSink.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/source/AbstractSource.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/source/SequenceGeneratorSource.java > 1351026 > trunk/flume-ng-core/src/main/java/org/apache/flume/source/StressSource.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > 1351026 > > trunk/flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java > 1351026 > > trunk/flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java > 1351026 > > trunk/flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java > 1351026 > > Diff: https://reviews.apache.org/r/5349/diff/ > > > Testing > ------- > > Yes, unit tested. > > > Thanks, > > Mubarak Seyed > >
