> On March 21, 2017, 6:25 a.m., Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java > > Line 433 (original), 382 (patched) > > <https://reviews.apache.org/r/57632/diff/3/?file=1669353#file1669353line436> > > > > 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.
Done. Let me know if the updated code comes close to what you had in mind. > On March 21, 2017, 6:25 a.m., Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java > > Line 480 (original), 434 (patched) > > <https://reviews.apache.org/r/57632/diff/3/?file=1669353#file1669353line510> > > > > 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. Done - was relying on IllegalArgException to be thrown earlier; updated it to explictly handle it by logging the error and rethrowing. > On March 21, 2017, 6:25 a.m., Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java > > Lines 1 (patched) > > <https://reviews.apache.org/r/57632/diff/3/?file=1669357#file1669357line1> > > > > Missing ASF license header. Thanks for catching this. Meant to update it in all the files :( > On March 21, 2017, 6:25 a.m., Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java > > Lines 30 (patched) > > <https://reviews.apache.org/r/57632/diff/3/?file=1669357#file1669357line30> > > > > Is there a reason why you didn't import java.util.Timer? I moved this code from CodahaleMetrics class and did not focus on the specific logic earlier. I have cleaned up the code a bit now - and moved away from using a Timer as well - using executor service instead. > On March 21, 2017, 6:25 a.m., Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > > Line 2211 (original), 2216 (patched) > > <https://reviews.apache.org/r/57632/diff/3/?file=1669359#file1669359line2216> > > > > Default value is 5s, but TimeValidator is in milliseconds. Mistake? Good catch! It looks like TimeValidator wasn't actually checking much as no min/max values were not specified. In any case its inconsistent and I've updated the default value to reflect the right metrics. - Sunitha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57632/#review169533 ----------------------------------------------------------- On March 21, 2017, 4:05 p.m., Sunitha Beeram wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57632/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 4:05 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/4/ > > > Testing > ------- > > Updated unit tests and all unit tests passed locally. > > > Thanks, > > Sunitha Beeram > >
