----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52084/#review150591 -----------------------------------------------------------
Looks mostly great, thanks for doing this. Also agree with Mohit on improving the logs, and had these two further suggestions. common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 192) <https://reviews.apache.org/r/52084/#comment218620> Should we not do anything instead of setting increment? If it is already corrupt I think we should just opt for the simplest solution, else it might lead for more confusion. common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 214) <https://reviews.apache.org/r/52084/#comment218621> Same for this line. In fact here I think it makes even less sense, if we decrement by 1, we would be setting to one. - Szehon Ho On Sept. 21, 2016, 3:20 p.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, 3:20 p.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 > >
