> On Sept. 21, 2016, 12:16 p.m., Gabor Szadovszky wrote: > > 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...)
I had the following cases: - getStoredScope used to throw IOException, but now it is throwing IllegalArgumentException. It is not part of the MetricScope API, but it is public, so if someone explicitly casts MetricScope to the implementing class then it can be accessed. Currently I cannot find it being used in the project except in tests. - getTimer is now throwing IllegalStateException instead of IOException. However incrementCounter and decrementCounter were both throwing RuntimeExpception for the same case (when the cache could not create the underlying object), so I thought it better to have the same behaviour. > On Sept. 21, 2016, 12:16 p.m., Gabor Szadovszky wrote: > > common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java, > > line 249 > > <https://reviews.apache.org/r/52084/diff/1/?file=1507094#file1507094line249> > > > > throws clause is not required here As this is a kind of public API I would like to keep the throws clause for documentation purposes. - Barna Zsombor ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52084/#review149810 ----------------------------------------------------------- On Sept. 21, 2016, 3:19 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:19 p.m.) > > > Review request for hive, Gabor Szadovszky, Peter Vary, Sergio Pena, and > Szehon Ho. > > > Repository: hive-git > > > Description > ------- > > HIVE-14775: Investigate IOException usage in Metrics APIs > > > 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 > >
