----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5145/#review7967 -----------------------------------------------------------
Mubarak. This is a big change. Good job. I have some quick reviews inline. Overall, besides the comments inline: - please create the diff by add --ignore-space-at-eol I was surprised to see we need to add so many code for configuration. Not sure we can reduce the work or not. Will look into more details. trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/DefaultMonitoringFactory.java <https://reviews.apache.org/r/5145/#comment17308> Do we really need the method? I think this class will be accessed through the interface. trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricAware.java <https://reviews.apache.org/r/5145/#comment17307> A little bit comments? trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricCollector.java <https://reviews.apache.org/r/5145/#comment17310> Better to make it configurable. trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricCollector.java <https://reviews.apache.org/r/5145/#comment17309> No need to synchronized? trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricContext.java <https://reviews.apache.org/r/5145/#comment17311> Not sure counterGroup is okay or not. It's only for counters. As metrics, we may also need to support float, like success/fail rate. trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/plugin/Ganglia31Monitoring.java <https://reviews.apache.org/r/5145/#comment17312> Here I have a comment for the directory layout: how about remove the plugin directory which is for all plugins? We can have individual directories for each plugin: flume-ng-core/src/main/java/org/apache/flume/metrics/ganglia/ flume-ng-core/src/main/java/org/apache/flume/metrics/json/ ... It would be the same convention of hadoop. trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/plugin/Ganglia31Monitoring.java <https://reviews.apache.org/r/5145/#comment17313> Remove the commented code? trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/plugin/Ganglia3Monitoring.java <https://reviews.apache.org/r/5145/#comment17314> Fix the todo's? or leave them later to fix? trunk/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java <https://reviews.apache.org/r/5145/#comment17315> As you mentioned at the very beginning, by default all sink/source/channel should implement MetricAware. So that we don't need all the (instance of ...) checks later on. trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/5145/#comment17316> As mentioned above, we don't have to do the check. trunk/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java <https://reviews.apache.org/r/5145/#comment17317> how about: public HDFSEventSink() { this(new HDFSWriterFactory()); } - Mingjie On 2012-05-17 05:24:59, Mubarak Seyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5145/ > ----------------------------------------------------------- > > (Updated 2012-05-17 05:24:59) > > > Review request for Flume. > > > Summary > ------- > > Code review for "Create metric collection infrastructure". This patch adds > fixes for the following > > - Added MetricAware interface, MetricContext data-structure, MetricCollector > - Implemented MetricAware in AvroSource, HDFSEventSink, and AvroSink > - Monitoring plugin processor: Added support in application bootstrap code to > parse and configure monitoring plugin(s) > - Monitoring plugin implementation: Added Ganglia 3.0 and 3.1 plugin to emit > metrics to gmond host:port > > Configuration for monitoring plugin: > ------------------------------- > agent.monitoring = ganglia_monitoring jmx_monitoring > > # plugin type: ganglia31 for version 3.1 compatible, ganglia3 for version 3.0 > compatible and wire protocol is different for both versions. > agent.monitoring.ganglia_monitoring.type = ganglia31 > # hostname where gmond daemon runs to collect and persist metrics in RRD > agent.monitoring.ganglia_monitoring.hostName = ganglia_host > # gmond port > agent.monitoring.ganglia_monitoring.port = 8649 > # polls every x seconds to collect metrics from metrics collector and emit > them to gmond > agent.monitoring.ganglia_monitoring.pollingInterval = 5 > > agent.monitoring.jmx_monitoring.type = jmx > agent.monitoring.jmx_monitoring.port = 8081 > > TODO: > ------ > - Convert all the sources, channels, sinks to use MetricAware interface and > let them be polled by MetricCollector > - Aggregation of metrics in collector side? Most of the counters uses > AtomicLong and increments the value upon success/failure, how do we aggregare > for minutes, hours? I think monitoring solution does the aggregation (e.g, > ganglia) > - MetricContext: Different data type (Double, Float) to store decimal values. > - Metrics: Other than counts, what else we needed? JVM metrics (we can get > from java.lang.management MXBeans) > - Channel: We can keep track of capacity, number of puts, number of takes, > remaining capacity (or exception count?) > - Monitoring plugins: JMX, REST > > Thanks, > Mubarak > > > Diffs > ----- > > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/BasicConfigurationConstants.java > 1338454 > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfiguration.java > 1338454 > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/ComponentConfigurationFactory.java > 1338454 > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java > 1338454 > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/monitoring/MonitoringConfiguration.java > PRE-CREATION > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/monitoring/MonitoringType.java > PRE-CREATION > trunk/flume-ng-core/pom.xml 1338454 > trunk/flume-ng-core/src/main/java/org/apache/flume/Monitoring.java > PRE-CREATION > trunk/flume-ng-core/src/main/java/org/apache/flume/MonitoringFactory.java > PRE-CREATION > > trunk/flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > 1338454 > > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/DefaultMonitoringFactory.java > PRE-CREATION > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricAware.java > PRE-CREATION > > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricCollector.java > PRE-CREATION > > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/MetricContext.java > PRE-CREATION > > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/plugin/Ganglia31Monitoring.java > PRE-CREATION > > trunk/flume-ng-core/src/main/java/org/apache/flume/metrics/plugin/Ganglia3Monitoring.java > PRE-CREATION > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java > 1338454 > > trunk/flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java > 1338454 > trunk/flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java > 1338530 > trunk/flume-ng-core/src/test/java/org/apache/flume/channel/MockChannel.java > 1338454 > > trunk/flume-ng-node/src/main/java/org/apache/flume/conf/file/AbstractFileConfigurationProvider.java > 1338454 > > trunk/flume-ng-node/src/main/java/org/apache/flume/conf/file/SimpleNodeConfiguration.java > 1338454 > > trunk/flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java > 1338454 > trunk/flume-ng-node/src/main/java/org/apache/flume/node/Application.java > 1338454 > > trunk/flume-ng-node/src/main/java/org/apache/flume/node/NodeConfiguration.java > 1338454 > > trunk/flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/DefaultLogicalNodeManager.java > 1338454 > > trunk/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 1338454 > > Diff: https://reviews.apache.org/r/5145/diff > > > Testing > ------- > > Yes. Tested in lab environment. > > > Thanks, > > Mubarak > >
