You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/10/13 23:18:43 UTC
Review Request 62995: HIVE-17806 Create directory for metrics file if
it doesn't exist
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/
-----------------------------------------------------------
Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
Bugs: HIVE-17806
https://issues.apache.org/jira/browse/HIVE-17806
Repository: hive-git
Description
-------
HIVE-17806 Create directory for metrics file if it doesn't exist
Diffs
-----
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
Diff: https://reviews.apache.org/r/62995/diff/1/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/
-----------------------------------------------------------
(Updated Oct. 19, 2017, 2 a.m.)
Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
Changes
-------
Create temp directory for the test if it doesn't exist
Summary (updated)
-----------------
HIVE-17806 Create directory for metrics file if it doesn't exist
Bugs: HIVE-17806
https://issues.apache.org/jira/browse/HIVE-17806
Repository: hive-git
Description (updated)
-------
HIVE-17806 Create directory for metrics file if it doesn't exist
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
Diff: https://reviews.apache.org/r/62995/diff/4/
Changes: https://reviews.apache.org/r/62995/diff/3-4/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 62995: HIVE-17806: Create directory for metrics
file if it doesn't exist
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188551
-----------------------------------------------------------
Ship it!
Ship It!
- Vihang Karajgaonkar
On Oct. 18, 2017, 7:07 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2017, 7:07 p.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806: Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62995: HIVE-17806: Create directory for metrics
file if it doesn't exist
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/
-----------------------------------------------------------
(Updated Oct. 18, 2017, 7:07 p.m.)
Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
Changes
-------
Addressed code review comment from Vihang
Summary (updated)
-----------------
HIVE-17806: Create directory for metrics file if it doesn't exist
Bugs: HIVE-17806
https://issues.apache.org/jira/browse/HIVE-17806
Repository: hive-git
Description (updated)
-------
HIVE-17806: Create directory for metrics file if it doesn't exist
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
Diff: https://reviews.apache.org/r/62995/diff/3/
Changes: https://reviews.apache.org/r/62995/diff/2-3/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Oct. 17, 2017, 11:21 p.m., Vihang Karajgaonkar wrote:
> > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/62995/diff/2/?file=1858825#file1858825line119>
> >
> > I think it is better to throw a RuntimeException here instead of catching it since there is nothing more that you can do if the metricsDir could not be created and it doesn't exist.
>
> Alexander Kolbasov wrote:
> Here we actually avoid creating JSON reporter. My thinking is that it isn't a catastrophic faiilure so you should be able to continue running without it. Do you think that it is better to throw an exception rather then continue running without the reporter?
>
> Vihang Karajgaonkar wrote:
> If we catch the exception and return then the ExecutorService remains uninitialized and when CodahaleMetrics class tries to close it sometime later it will result in a NPE on ExecutorService.shutdown(). I think throwing this exception and handling it at CodahaleMetrics such that this reporter is not added in list of reporters would be cleaner way to handle this error.
Added a check in close() to avoid NPE.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188416
-----------------------------------------------------------
On Oct. 17, 2017, 12:35 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 12:35 a.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806 Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Oct. 17, 2017, 11:21 p.m., Vihang Karajgaonkar wrote:
> > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/62995/diff/2/?file=1858825#file1858825line119>
> >
> > I think it is better to throw a RuntimeException here instead of catching it since there is nothing more that you can do if the metricsDir could not be created and it doesn't exist.
Here we actually avoid creating JSON reporter. My thinking is that it isn't a catastrophic faiilure so you should be able to continue running without it. Do you think that it is better to throw an exception rather then continue running without the reporter?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188416
-----------------------------------------------------------
On Oct. 17, 2017, 12:35 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 12:35 a.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806 Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
> On Oct. 17, 2017, 11:21 p.m., Vihang Karajgaonkar wrote:
> > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/62995/diff/2/?file=1858825#file1858825line119>
> >
> > I think it is better to throw a RuntimeException here instead of catching it since there is nothing more that you can do if the metricsDir could not be created and it doesn't exist.
>
> Alexander Kolbasov wrote:
> Here we actually avoid creating JSON reporter. My thinking is that it isn't a catastrophic faiilure so you should be able to continue running without it. Do you think that it is better to throw an exception rather then continue running without the reporter?
If we catch the exception and return then the ExecutorService remains uninitialized and when CodahaleMetrics class tries to close it sometime later it will result in a NPE on ExecutorService.shutdown(). I think throwing this exception and handling it at CodahaleMetrics such that this reporter is not added in list of reporters would be cleaner way to handle this error.
- Vihang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188416
-----------------------------------------------------------
On Oct. 17, 2017, 12:35 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 12:35 a.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806 Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188416
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Lines 119 (patched)
<https://reviews.apache.org/r/62995/#comment265417>
I think it is better to throw a RuntimeException here instead of catching it since there is nothing more that you can do if the metricsDir could not be created and it doesn't exist.
- Vihang Karajgaonkar
On Oct. 17, 2017, 12:35 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 12:35 a.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806 Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 12:35 a.m.)
Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
Changes
-------
Addressed code review comments from Andrew
Bugs: HIVE-17806
https://issues.apache.org/jira/browse/HIVE-17806
Repository: hive-git
Description
-------
HIVE-17806 Create directory for metrics file if it doesn't exist
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
Diff: https://reviews.apache.org/r/62995/diff/2/
Changes: https://reviews.apache.org/r/62995/diff/1-2/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 62995: HIVE-17806 Create directory for metrics
file if it doesn't exist
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62995/#review188223
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 89 (original), 92 (patched)
<https://reviews.apache.org/r/62995/#comment265238>
comment refers to old name
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 102 (original), 105 (patched)
<https://reviews.apache.org/r/62995/#comment265240>
should this say "in" the same directory ?
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 173 (original), 188 (patched)
<https://reviews.apache.org/r/62995/#comment265241>
should be "temporary" not yemporary
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
Line 164 (original), 179 (patched)
<https://reviews.apache.org/r/62995/#comment265243>
should be"temporary"
- Andrew Sherman
On Oct. 13, 2017, 11:18 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62995/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2017, 11:18 p.m.)
>
>
> Review request for hive, Aihua Xu, Andrew Sherman, Janaki Lahorani, Sergio Pena, Sahil Takiar, and Vihang Karajgaonkar.
>
>
> Bugs: HIVE-17806
> https://issues.apache.org/jira/browse/HIVE-17806
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-17806 Create directory for metrics file if it doesn't exist
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java 96243cb74a154b9a639ffb080256c4b43bd35a4b
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 254af7d4310578e3883c0dffa64bed0f823696ea
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java f71bb25463b56bc741f989454664397996b6a5cf
>
>
> Diff: https://reviews.apache.org/r/62995/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>