----------------------------------------------------------- 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 > >
