Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-27 Thread Alexander Kolbasov 


> On Sept. 25, 2017, 7:05 p.m., Sahil Takiar wrote:
> > Changes look good. Just to confirm, the changes to the unit tests include 
> > an explicit test for the bug reported in HIVE-17563, correct? IOW, there is 
> > a test that first writes the metrics files, updates the metrics in HS2, 
> > re-writes the metrics file, and then validates that the re-written metrics 
> > file contains the updated values?

The tests for both classes do include the test that verifies that the JSON file 
is updated every time the metric is updated along the lines that you described.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/#review186149
---


On Sept. 22, 2017, 11:46 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62487/
> ---
> 
> (Updated Sept. 22, 2017, 11:46 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Carl Steinbach, Alan Gates, Sergio Pena, 
> Sahil Takiar, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-17563
> https://issues.apache.org/jira/browse/HIVE-17563
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
> hive.service.metrics.file.location
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  c07517a634e35c936d6ea68e9a2874ac2c59929a 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  67f81d6c43a7904dc384cfcce6f948f1f802144c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  b804cdade07079955a65cb431fab078dcecd53b1 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
>  259a4db43905c47d873776df96210a4e77d07076 
> 
> 
> Diff: https://reviews.apache.org/r/62487/diff/5/
> 
> 
> Testing
> ---
> 
> TestCodahaleMetrics tests that the json reporter functionality
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-25 Thread Sahil Takiar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/#review186149
---


Ship it!




Changes look good. Just to confirm, the changes to the unit tests include an 
explicit test for the bug reported in HIVE-17563, correct? IOW, there is a test 
that first writes the metrics files, updates the metrics in HS2, re-writes the 
metrics file, and then validates that the re-written metrics file contains the 
updated values?

- Sahil Takiar


On Sept. 22, 2017, 11:46 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62487/
> ---
> 
> (Updated Sept. 22, 2017, 11:46 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Carl Steinbach, Alan Gates, Sergio Pena, 
> Sahil Takiar, and Vihang Karajgaonkar.
> 
> 
> Bugs: HIVE-17563
> https://issues.apache.org/jira/browse/HIVE-17563
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
> hive.service.metrics.file.location
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  c07517a634e35c936d6ea68e9a2874ac2c59929a 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  67f81d6c43a7904dc384cfcce6f948f1f802144c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  b804cdade07079955a65cb431fab078dcecd53b1 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
>  259a4db43905c47d873776df96210a4e77d07076 
> 
> 
> Diff: https://reviews.apache.org/r/62487/diff/5/
> 
> 
> Testing
> ---
> 
> TestCodahaleMetrics tests that the json reporter functionality
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

(Updated Sept. 22, 2017, 11:46 p.m.)


Review request for hive, Aihua Xu, Carl Steinbach, Alan Gates, Sergio Pena, 
Sahil Takiar, and Vihang Karajgaonkar.


Changes
---

Addressed comments from Sahil


Bugs: HIVE-17563
https://issues.apache.org/jira/browse/HIVE-17563


Repository: hive-git


Description
---

HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location


Diffs (updated)
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634e35c936d6ea68e9a2874ac2c59929a 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43a7904dc384cfcce6f948f1f802144c 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
 b804cdade07079955a65cb431fab078dcecd53b1 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
 259a4db43905c47d873776df96210a4e77d07076 


Diff: https://reviews.apache.org/r/62487/diff/5/

Changes: https://reviews.apache.org/r/62487/diff/4-5/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Alexander Kolbasov 


> On Sept. 22, 2017, 7:48 p.m., Sahil Takiar wrote:
> > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
> > Line 75 (original), 116 (patched)
> > 
> >
> > I think the old implementation called `shutdown()` instead of 
> > `shutdownNow()`. It may be best to call `shutdown()` first, call 
> > `awaitTermination(30s)`, and then call `shutdownNow()`.

Reverted to the old code (calling shutdown())


> On Sept. 22, 2017, 7:48 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
> > Lines 107 (patched)
> > 
> >
> > Might want to change this to something more informative, like `logging 
> > metrics with frequency: `

Removed.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/#review186011
---


On Sept. 22, 2017, 6:14 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62487/
> ---
> 
> (Updated Sept. 22, 2017, 6:14 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-17563
> https://issues.apache.org/jira/browse/HIVE-17563
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
> hive.service.metrics.file.location
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  c07517a634e35c936d6ea68e9a2874ac2c59929a 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  67f81d6c43a7904dc384cfcce6f948f1f802144c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  b804cdade07079955a65cb431fab078dcecd53b1 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
>  259a4db43905c47d873776df96210a4e77d07076 
> 
> 
> Diff: https://reviews.apache.org/r/62487/diff/4/
> 
> 
> Testing
> ---
> 
> TestCodahaleMetrics tests that the json reporter functionality
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Sahil Takiar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/#review186011
---




common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 75 (original), 116 (patched)


I think the old implementation called `shutdown()` instead of 
`shutdownNow()`. It may be best to call `shutdown()` first, call 
`awaitTermination(30s)`, and then call `shutdownNow()`.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 77 (original), 118 (patched)


I don't know if we want this file to be deleted. The `close()` method is 
basically invoked when HS2 is shutdown, but there may be external systems who 
read the metrics after HS2 has been shutdown.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Lines 157 (patched)


This looks like it should go into a `finally` block.



common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 118 (original), 166 (patched)


This looks like it should go into a `finally` block.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
Lines 107 (patched)


Might want to change this to something more informative, like `logging 
metrics with frequency: `


- Sahil Takiar


On Sept. 22, 2017, 6:14 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62487/
> ---
> 
> (Updated Sept. 22, 2017, 6:14 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-17563
> https://issues.apache.org/jira/browse/HIVE-17563
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
> hive.service.metrics.file.location
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  c07517a634e35c936d6ea68e9a2874ac2c59929a 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  67f81d6c43a7904dc384cfcce6f948f1f802144c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  b804cdade07079955a65cb431fab078dcecd53b1 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
>  259a4db43905c47d873776df96210a4e77d07076 
> 
> 
> Diff: https://reviews.apache.org/r/62487/diff/4/
> 
> 
> Testing
> ---
> 
> TestCodahaleMetrics tests that the json reporter functionality
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

(Updated Sept. 22, 2017, 6:14 p.m.)


Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
Karajgaonkar.


Changes
---

- Added comments to address code review comments from Vihang.


Bugs: HIVE-17563
https://issues.apache.org/jira/browse/HIVE-17563


Repository: hive-git


Description
---

HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location


Diffs (updated)
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634e35c936d6ea68e9a2874ac2c59929a 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43a7904dc384cfcce6f948f1f802144c 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
 b804cdade07079955a65cb431fab078dcecd53b1 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
 259a4db43905c47d873776df96210a4e77d07076 


Diff: https://reviews.apache.org/r/62487/diff/4/

Changes: https://reviews.apache.org/r/62487/diff/3-4/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Vihang Karajgaonkar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/#review186000
---




common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
Line 53 (original), 68 (patched)


Its not clear looking at the code why we are using java.nio.file.Path 
instead of org.apache.hadoop.fs.Path. Can you please add a comment explain the 
same?


- Vihang Karajgaonkar


On Sept. 22, 2017, 4:01 p.m., Alexander Kolbasov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62487/
> ---
> 
> (Updated Sept. 22, 2017, 4:01 p.m.)
> 
> 
> Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-17563
> https://issues.apache.org/jira/browse/HIVE-17563
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
> hive.service.metrics.file.location
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
>  c07517a634e35c936d6ea68e9a2874ac2c59929a 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
>  67f81d6c43a7904dc384cfcce6f948f1f802144c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
>  b804cdade07079955a65cb431fab078dcecd53b1 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
>  259a4db43905c47d873776df96210a4e77d07076 
> 
> 
> Diff: https://reviews.apache.org/r/62487/diff/3/
> 
> 
> Testing
> ---
> 
> TestCodahaleMetrics tests that the json reporter functionality
> 
> 
> Thanks,
> 
> Alexander Kolbasov
> 
>

Re: Review Request 62487: HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

(Updated Sept. 22, 2017, 4:01 p.m.)


Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
Karajgaonkar.


Changes
---

removed unused imports


Summary (updated)
-

HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location


Bugs: HIVE-17563
https://issues.apache.org/jira/browse/HIVE-17563


Repository: hive-git


Description (updated)
---

HIVE-17563: CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location


Diffs (updated)
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634e35c936d6ea68e9a2874ac2c59929a 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43a7904dc384cfcce6f948f1f802144c 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
 b804cdade07079955a65cb431fab078dcecd53b1 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
 259a4db43905c47d873776df96210a4e77d07076 


Diff: https://reviews.apache.org/r/62487/diff/3/

Changes: https://reviews.apache.org/r/62487/diff/2-3/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov

Re: Review Request 62487: HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-22 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

(Updated Sept. 22, 2017, 7:25 a.m.)


Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
Karajgaonkar.


Changes
---

Fixed JsonReporter and its test


Bugs: HIVE-17563
https://issues.apache.org/jira/browse/HIVE-17563


Repository: hive-git


Description
---

HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location

A few other changes included:

- Test clean up
- provide useful name for JSON reporter thread


Diffs (updated)
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/JsonReporter.java
 b804cdade0 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java
 259a4db439 


Diff: https://reviews.apache.org/r/62487/diff/2/

Changes: https://reviews.apache.org/r/62487/diff/1-2/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov

Review Request 62487: HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-21 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
Karajgaonkar.


Repository: hive-git


Description
---

HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location

A few other changes included:

- Test clean up
- provide useful name for JSON reporter thread


Diffs
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634e35c936d6ea68e9a2874ac2c59929a 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43a7904dc384cfcce6f948f1f802144c 


Diff: https://reviews.apache.org/r/62487/diff/1/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov

Re: Review Request 62487: HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating hive.service.metrics.file.location

2017-09-21 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62487/
---

(Updated Sept. 21, 2017, 11:49 p.m.)


Review request for hive, Aihua Xu, Sergio Pena, Sahil Takiar, and Vihang 
Karajgaonkar.


Bugs: HIVE-17563
https://issues.apache.org/jira/browse/HIVE-17563


Repository: hive-git


Description
---

HIVE-17563 CodahaleMetrics.JsonFileReporter is not updating 
hive.service.metrics.file.location

A few other changes included:

- Test clean up
- provide useful name for JSON reporter thread


Diffs
-

  
common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/JsonFileMetricsReporter.java
 c07517a634e35c936d6ea68e9a2874ac2c59929a 
  
common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java
 67f81d6c43a7904dc384cfcce6f948f1f802144c 


Diff: https://reviews.apache.org/r/62487/diff/1/


Testing
---

TestCodahaleMetrics tests that the json reporter functionality


Thanks,

Alexander Kolbasov