> On March 22, 2017, 8:31 p.m., Vihang Karajgaonkar wrote: > > Thanks for the changes Sunitha. Few comments below.
Thanks for reviewing! Appreciate it. > On March 22, 2017, 8:31 p.m., Vihang Karajgaonkar wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java > > Line 392 (original), 381 (patched) > > <https://reviews.apache.org/r/57632/diff/4/?file=1670926#file1670926line395> > > > > Does code does not fallback to initMetricsReported() if > > initCodahaleMetricsReporterClasses() throws an exception? I would like to fall back to the deprecated config only if the new one is not available - in which case the method returns false (not an exception). But if it is present and we encounter issues with it, I would like to not fall back at that time, ie, the intent is only backwards compatibility and not to support multiple confs at the same time. Let me know if you think otherwise. > On March 22, 2017, 8:31 p.m., Vihang Karajgaonkar wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleReporter.java > > Lines 22 (patched) > > <https://reviews.apache.org/r/57632/diff/4/?file=1670927#file1670927line22> > > > > I think this can be renamed to something more generic like > > MetricsReporter. Looks like I regressed in my updates, the original signature was interface CodahaleReporter extends Reporter, Closeable It was extending from a Codahale specific reporter (similar to ScheduledReporter) and I intended this to be used for pluggable codahale reporters as most things here are specific to Codahale. I plan to bring that signature back, unless there is a strong inclination for keeping it generic. (I currently don't have much driving the generic use-case). > On March 22, 2017, 8:31 p.m., Vihang Karajgaonkar wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > > Lines 2213 (patched) > > <https://reviews.apache.org/r/57632/diff/4/?file=1670932#file1670932line2213> > > > > If you intent to deprecate this config use @deprecated instead. Also, > > instead of "- will be .. " it would be good if it says "This configuration > > will be ..." Good point. Will do. > On March 22, 2017, 8:31 p.m., Vihang Karajgaonkar wrote: > > common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java > > Line 62 (original), 63 (patched) > > <https://reviews.apache.org/r/57632/diff/4/?file=1670933#file1670933line63> > > > > Can you add additional testcases to verify that older config gets used > > when the newer one is not set? Surely. I am currently working on adding it - the current test code sets up the reporters in the before method and entails some more work to cleanly test for backwards compatibility, so wanted to embark on it once the approach was agreed on. Will include it in my next update. - Sunitha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57632/#review169775 ----------------------------------------------------------- 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 > >
