hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Carl Steinbach <...@apache.org>
Subject Re: Review Request 57632: HIVE-16206: Provide wrapper classes for current metrics reporters to allow uniform instantiation through reflection
Date Tue, 21 Mar 2017 06:25:32 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57632/#review169533
-----------------------------------------------------------




common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java
Line 433 (original), 382 (patched)
<https://reviews.apache.org/r/57632/#comment241912>

    Please consider pushing this logic into the initReportingByClasses() and initReportyByEnum()
methods, and changing the names of these methods to reference the names of the corresponding
configuration properties.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java
Line 450 (original), 401 (patched)
<https://reviews.apache.org/r/57632/#comment241911>

    Error message should reference the name of the configuration property that contains the
bad value.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java
Line 480 (original), 434 (patched)
<https://reviews.apache.org/r/57632/#comment241910>

    If the reporterName is not recognized it would be good to either log an error/warning
or throw an exception. The error message should reference the name of the configuration property
that contains the unrecognized value.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Lines 1 (patched)
<https://reviews.apache.org/r/57632/#comment241914>

    Missing ASF license header.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Lines 30 (patched)
<https://reviews.apache.org/r/57632/#comment241915>

    Is there a reason why you didn't import java.util.Timer?



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Lines 74 (patched)
<https://reviews.apache.org/r/57632/#comment241917>

    Include the file name in the log message. Also, should this be logged at the error level?



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/Metrics2Reporter.java
Lines 1 (patched)
<https://reviews.apache.org/r/57632/#comment241916>

    Missing ASF license header.



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Line 2204 (original), 2204 (patched)
<https://reviews.apache.org/r/57632/#comment241918>

    The config property descriptions should explain what happens when both HIVE_CODAHALE_METRICS_REPORTER_CLASSES
and HIVE_METRICS_REPORTER are set.



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
Line 2211 (original), 2216 (patched)
<https://reviews.apache.org/r/57632/#comment241919>

    Default value is 5s, but TimeValidator is in milliseconds. Mistake?



common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
Line 62 (original), 63 (patched)
<https://reviews.apache.org/r/57632/#comment241920>

    Add a linebreak.


- Carl Steinbach


On March 20, 2017, 3:44 p.m., Sunitha Beeram wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57632/
> -----------------------------------------------------------
> 
> (Updated March 20, 2017, 3:44 p.m.)
> 
> 
> Review request for hive, Carl Steinbach and Ratandeep Ratti.
> 
> 
> Bugs: Hive-16206
>     https://issues.apache.org/jira/browse/Hive-16206
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-16206: Address review comments
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java
e8abf6cf06afc9fa590af3a447eacc67735a69e6 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleReporter.java
PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/ConsoleMetricsReporter.java
PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JmxMetricsReporter.java
PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/Metrics2Reporter.java
PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1fb32533d58af4ec622feb320bf9315da5db6e76

>   common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
aa4e75f9f8160d1b54b14c1a23ea42e156bd45ca 
> 
> 
> Diff: https://reviews.apache.org/r/57632/diff/3/
> 
> 
> Testing
> -------
> 
> Updated unit tests and all unit tests passed locally.
> 
> 
> Thanks,
> 
> Sunitha Beeram
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message