You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Barna Zsombor Klara <zs...@cloudera.com> on 2016/09/21 09:24:08 UTC
Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/
-----------------------------------------------------------
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
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
> 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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Gabor Szadovszky <ga...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Gabor Szadovszky <ga...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/#review149832
-----------------------------------------------------------
Ship it!
Thanks, LGTM.
- Gabor Szadovszky
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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/#review150736
-----------------------------------------------------------
Ship it!
Ship It!
- Szehon Ho
On Sept. 28, 2016, 2:12 p.m., Barna Zsombor Klara wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52084/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2016, 2:12 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 71175dffb6f425e971e2a16aa2233de2622919a3
> ql/src/java/org/apache/hadoop/hive/ql/Driver.java 03c56e1d1838a5a239575a7fd8365dcc6787cce3
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapRedTask.java f48d5110bcc0d37a37fbeb8abb50dc17876b04af
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java f81fc71d893dea44766eaf89c81825cd728babf7
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 72c8bf792c2e5f2c14a7548ebfe7a0a7541e7115
> ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezTask.java c51c92f1fc168633d00b69992e321d4be193f460
> 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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/
-----------------------------------------------------------
(Updated Sept. 28, 2016, 2:12 p.m.)
Review request for hive, Gabor Szadovszky, Peter Vary, Sergio Pena, and Szehon Ho.
Changes
-------
Addressed the issues spotted by Mohit and Szehon and did a rebased to incorporate already committed changes. Thank you for the reviews!
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 (updated)
-----
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 71175dffb6f425e971e2a16aa2233de2622919a3
ql/src/java/org/apache/hadoop/hive/ql/Driver.java 03c56e1d1838a5a239575a7fd8365dcc6787cce3
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapRedTask.java f48d5110bcc0d37a37fbeb8abb50dc17876b04af
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java f81fc71d893dea44766eaf89c81825cd728babf7
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkTask.java 72c8bf792c2e5f2c14a7548ebfe7a0a7541e7115
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezTask.java c51c92f1fc168633d00b69992e321d4be193f460
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
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/#review149828
-----------------------------------------------------------
LGTM, thanks for the patch
- Peter Vary
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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/#review150575
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 93)
<https://reviews.apache.org/r/52084/#comment218598>
Log numCounter in the string so we know which counter value could not be found.
Same for other places where you're handling JMException and know what metric is getting logged.
common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 191)
<https://reviews.apache.org/r/52084/#comment218599>
same for this file.
- Mohit Sabharwal
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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
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 (updated)
-------
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
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Added a unit test, reduced code duplication and answered comments.
Thank you for the reviews.
Repository: hive-git
Description (updated)
-------
HIVE-14775: Investigate IOException usage in Metrics APIs
Diffs (updated)
-----
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
Re: Review Request 52084: HIVE-14775: Investigate IOException usage in
Metrics APIs
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52084/#review149805
-----------------------------------------------------------
Thanks Zsombor!
Godd refactor, just minor nits, and some questions.
Thanks,
Peter
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java (line 262)
<https://reviews.apache.org/r/52084/#comment217526>
nit: retrieving
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java (line 281)
<https://reviews.apache.org/r/52084/#comment217527>
nit: retrieving
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java (line 318)
<https://reviews.apache.org/r/52084/#comment217528>
nit: retrieving
common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java
<https://reviews.apache.org/r/52084/#comment217529>
Not really into this part of the code, but we might want at least check that calling the startStoredScope again will not cause any trouble here. Would be perfect if we could check the log, but that might be an overkill :)
metastore/src/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java (lines 48 - 50)
<https://reviews.apache.org/r/52084/#comment217530>
If refactoring, this 3 lines might be put into an util method, to avoid code duplication :). Not that important, just an idea
- Peter Vary
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
>
>