You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Szehon Ho <sz...@cloudera.com> on 2015/07/07 05:00:32 UTC

Review Request 36241: HIVE-10927 : Add number of HMS connection metrics

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

Review request for hive.


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


Repository: hive-git


Description
-------

Adds following new metrics to HMS, can be renamed if necessary.

"open_connections";  //#HMS clients connecting to HMS

"active_jdo_transactions";  //#active JDO transactions
"rollbacked_jdo_transactions";  //#failed JDO transactions
"committed_jdo_transactions";  //#successful JDO transactions
"opened_jdo_transactions";  //#attempted JDO transactions

Also to HS2:
"open_connections";  //#HMS clients connecting to HMS


This also fixes some minor issues:
1.  For the metrics JSON-file-reporter, the file system was not chosen right.  Fixing that, and also making the default the local file system.
2.  The metrics was getting closed in the metastore in the wrong place, fixing it.


Diffs
-----

  common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java ec5ac4a 
  common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java e811339 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java 27b69cc 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsVariable.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java ae353d0 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d0cf15 
  common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 954b388 
  itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMetrics.java 25f34d1 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 0bcd053 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 4273c0b 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java dfb7faa 

Diff: https://reviews.apache.org/r/36241/diff/


Testing
-------

Adding some unit tests, and tested manually for HS2 which cannot be unit-tested.


Thanks,

Szehon Ho


Re: Review Request 36241: HIVE-10927 : Add number of HMS connection metrics

Posted by Jimmy Xiang <jx...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36241/#review90856
-----------------------------------------------------------

Ship it!


Ship It!

- Jimmy Xiang


On July 7, 2015, 9:28 p.m., Szehon Ho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36241/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 9:28 p.m.)
> 
> 
> Review request for hive and Jimmy Xiang.
> 
> 
> Bugs: HIVE-10927
>     https://issues.apache.org/jira/browse/HIVE-10927
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Adds following new metrics to HMS, can be renamed if necessary.
> 
> "open_connections";  //#HMS clients connecting to HMS
> 
> "active_jdo_transactions";  //#active JDO transactions
> "rollbacked_jdo_transactions";  //#failed JDO transactions
> "committed_jdo_transactions";  //#successful JDO transactions
> "opened_jdo_transactions";  //#attempted JDO transactions
> 
> Also to HS2:
> "open_connections";  //#HMS clients connecting to HMS
> 
> 
> This also fixes some minor issues:
> 1.  For the metrics JSON-file-reporter, the file system was not chosen right.  Fixing that, and also making the default the local file system.
> 2.  The metrics was getting closed in the metastore in the wrong place, fixing it.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java ec5ac4a 
>   common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java e811339 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java 27b69cc 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsVariable.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java ae353d0 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d0cf15 
>   common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 954b388 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMetrics.java 25f34d1 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 0bcd053 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 4273c0b 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java dfb7faa 
> 
> Diff: https://reviews.apache.org/r/36241/diff/
> 
> 
> Testing
> -------
> 
> Adding some unit tests, and tested manually for HS2 which cannot be unit-tested.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>


Re: Review Request 36241: HIVE-10927 : Add number of HMS connection metrics

Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36241/
-----------------------------------------------------------

(Updated July 7, 2015, 9:28 p.m.)


Review request for hive and Jimmy Xiang.


Changes
-------

Address the review comments.


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


Repository: hive-git


Description
-------

Adds following new metrics to HMS, can be renamed if necessary.

"open_connections";  //#HMS clients connecting to HMS

"active_jdo_transactions";  //#active JDO transactions
"rollbacked_jdo_transactions";  //#failed JDO transactions
"committed_jdo_transactions";  //#successful JDO transactions
"opened_jdo_transactions";  //#attempted JDO transactions

Also to HS2:
"open_connections";  //#HMS clients connecting to HMS


This also fixes some minor issues:
1.  For the metrics JSON-file-reporter, the file system was not chosen right.  Fixing that, and also making the default the local file system.
2.  The metrics was getting closed in the metastore in the wrong place, fixing it.


Diffs (updated)
-----

  common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java ec5ac4a 
  common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java e811339 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java 27b69cc 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsVariable.java PRE-CREATION 
  common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java ae353d0 
  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d0cf15 
  common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 954b388 
  itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMetrics.java 25f34d1 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 0bcd053 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 4273c0b 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java dfb7faa 

Diff: https://reviews.apache.org/r/36241/diff/


Testing
-------

Adding some unit tests, and tested manually for HS2 which cannot be unit-tested.


Thanks,

Szehon Ho


Re: Review Request 36241: HIVE-10927 : Add number of HMS connection metrics

Posted by Jimmy Xiang <jx...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36241/#review90760
-----------------------------------------------------------


Looks good to me. Just couple nits.


common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 188)
<https://reviews.apache.org/r/36241/#comment143887>

    Nit. variable name: decrement?



common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java (line 81)
<https://reviews.apache.org/r/36241/#comment143888>

    Same here, and javadoc. decrement?



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 6075)
<https://reviews.apache.org/r/36241/#comment143889>

    Should we use metrics instead of calling MetricsFactory.getInstance() again?
    
    Same below.



metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 270)
<https://reviews.apache.org/r/36241/#comment143892>

    Do we need to make openTransactionCalls volatile? This variable is accessed from multiple threads, right?


- Jimmy Xiang


On July 7, 2015, 3 a.m., Szehon Ho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36241/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 3 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-10927
>     https://issues.apache.org/jira/browse/HIVE-10927
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Adds following new metrics to HMS, can be renamed if necessary.
> 
> "open_connections";  //#HMS clients connecting to HMS
> 
> "active_jdo_transactions";  //#active JDO transactions
> "rollbacked_jdo_transactions";  //#failed JDO transactions
> "committed_jdo_transactions";  //#successful JDO transactions
> "opened_jdo_transactions";  //#attempted JDO transactions
> 
> Also to HS2:
> "open_connections";  //#HMS clients connecting to HMS
> 
> 
> This also fixes some minor issues:
> 1.  For the metrics JSON-file-reporter, the file system was not chosen right.  Fixing that, and also making the default the local file system.
> 2.  The metrics was getting closed in the metastore in the wrong place, fixing it.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java ec5ac4a 
>   common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java e811339 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java 27b69cc 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsVariable.java PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java ae353d0 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6d0cf15 
>   common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java 954b388 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMetrics.java 25f34d1 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 0bcd053 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 4273c0b 
>   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java dfb7faa 
> 
> Diff: https://reviews.apache.org/r/36241/diff/
> 
> 
> Testing
> -------
> 
> Adding some unit tests, and tested manually for HS2 which cannot be unit-tested.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>