----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34854/#review87567 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139951> As a convention, avoid using upper case names for member variables. They tend to be associated with static final / constants. core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139949> Consider using boolean isExternalMonitoringEnabled; core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139948> Consider adding stop() in this class and invoke it on MetricsInstrumentationService::destroy(). Stop to flush pending metrics and actually stop the reporters. core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139950> Use ConfigurationService::getBoolean() instead core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139952> This seems to be specific to Graphite. Can we default this to a constant value instead of accepting it from user as an external config? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139953> Sane default would be useful for this. core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139955> Potential NPE here core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139954> Potential NPE here core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139956> Is it acceptable to stop the Oozie server startup on failure to initialize the metrics system ? core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java <https://reviews.apache.org/r/34854/#comment139957> Possible to add any UTs ? Also add relevant documentation to help out administrators who would wire this up in real deployments. - Srikanth Sundarrajan On June 8, 2015, 12:12 p.m., Narayan Periwal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34854/ > ----------------------------------------------------------- > > (Updated June 8, 2015, 12:12 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2251 > https://issues.apache.org/jira/browse/OOZIE-2251 > > > Repository: oozie-git > > > Description > ------- > > We have been logging so many important matrices in oozie-instrumentation.log > . These information is very useful for oozie functional monitoring. But it is > always difficult to get the meaning from flat file. If we expose this > information on some graphing tool, We can get the lot of meaning out of it > and can take some actions based on it. > > > Diffs > ----- > > core/pom.xml 7877773 > core/src/main/java/org/apache/oozie/util/MetricsInstrumentation.java > e56bfda > core/src/main/resources/oozie-default.xml 8960073 > > Diff: https://reviews.apache.org/r/34854/diff/ > > > Testing > ------- > > Done > > > Thanks, > > Narayan Periwal > >
