----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52084/#review149810 -----------------------------------------------------------
Thanks for the patch. One minor finding and a question: At many places you have refactored to throw unchecked exceptions instead of checked ones while removed the catch blocks of the checked ones. Are you sure it cannot cause any problems in production? (E.g. metrics should never fail a query...) common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 244) <https://reviews.apache.org/r/52084/#comment217536> throws clause is not required here - Gabor Szadovszky On Sept. 21, 2016, 9:24 a.m., Barna Zsombor Klara wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52084/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2016, 9:24 a.m.) > > > Review request for hive, Gabor Szadovszky, Peter Vary, Sergio Pena, and > Szehon Ho. > > > Repository: hive-git > > > Description > ------- > > I have refactored the exception handling in the metrics API. The initial > objective was to remove the throws clause from the methods which did not > throw IOExceptions but were declaring it. Finally I continued to remove the > throws clause from the rest of the APIs since we were never able to do > anything else with the excpetions except log them out. New log messages were > introduced inside the metrics classes to capture exceptional conditions so we > can omit the try catch blocks when creating/updating metrics. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java > 9be9b50aa02ff88816eb92079eaff9afa3e1be90 > common/src/java/org/apache/hadoop/hive/common/metrics/MetricsMBean.java > 19946d9e93d55bbffb6d03cbc35e569849a86dd8 > common/src/java/org/apache/hadoop/hive/common/metrics/MetricsMBeanImpl.java > a973155f079c6124a6981b04123d9496dc5d3448 > common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java > 4297233ed12a7d9a2fa03ac3204e8335c0aed821 > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java > 4c433678bd62ea74b80babce9856681192deb25f > common/src/java/org/apache/hadoop/hive/ql/log/PerfLogger.java > 6a5d22f2bcc0a7efd869f7c0c0c8fad33ca35f74 > > common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java > a3fb04f1ab9be8be9f69c616eabeb534b2a2d560 > metastore/src/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java > 6830cf75dc163230cdabb4ed41d65c222c6ca54d > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > f0b84768e891e4281a613e68590524646a038435 > ql/src/java/org/apache/hadoop/hive/ql/Driver.java > 42d398dcc9a37bdb2d90c940c579c55fb76b4cca > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 90fe76d00e6f833e18d183c290f13c23db9303a1 > > Diff: https://reviews.apache.org/r/52084/diff/ > > > Testing > ------- > > Tested basic metric gathering with both codahale and legacy metrics class. > Updated and ran unit tests in the common project. > > > Thanks, > > Barna Zsombor Klara > >
