----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52684/#review151968 -----------------------------------------------------------
Hi Zsombor, Thanks for the great patch. Minor nits, and some questions - most probably some stuff is not clear for me :) Thanks, Peter common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java (line 27) <https://reviews.apache.org/r/52684/#comment220647> If we refactor this, isn't this better placed in MetricsConstant? common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java (line 104) <https://reviews.apache.org/r/52684/#comment220648> In your patch description it is 5/10/15 min :) Reading the Meter javadoc, 1/5/15 seems correct to me common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java (line 71) <https://reviews.apache.org/r/52684/#comment220645> Might be a good idea to have a comment here for the new metrics ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 579) <https://reviews.apache.org/r/52684/#comment220639> Why this MetricsQueryLifeTimeHook is not another one of the HIVE_QUERY_LIFETIME_HOOKS? ql/src/test/org/apache/hadoop/hive/ql/hooks/TestMetricsQueryLifeTimeHook.java (line 78) <https://reviews.apache.org/r/52684/#comment220640> Maybe this is just me not understanding the description for the metrics... You wrote this in the description: "- hs2_executing_queries - is showing the number of queries currently in the execution phase " After calling the beforeExecution method I think the count should be 1, since we have 1 query in the execution phase The same for HS2_COMPILING_QUERIES, if my comment above is valid. service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java (line 667) <https://reviews.apache.org/r/52684/#comment220641> nit: space after if service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java (line 670) <https://reviews.apache.org/r/52684/#comment220642> nit: space after if service/src/test/org/apache/hive/service/cli/operation/TestSQLOperationMetrics.java (line 52) <https://reviews.apache.org/r/52684/#comment220644> I am not sure that this is important but creating a new SessionState runs a quite big chunk of code. Is it possible to use only one SessionState in the test, and close() it at tearDown? - Peter Vary On Oct. 10, 2016, 12:16 p.m., Barna Zsombor Klara wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52684/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2016, 12:16 p.m.) > > > Review request for hive, Gabor Szadovszky, Marta Kuczora, Mohit Sabharwal, > Peter Vary, Sergio Pena, and Szehon Ho. > > > Repository: hive-git > > > Description > ------- > > HIVE-14754: Track the queries execution lifecycle times > Five new metrics are being added to track the query execution in HiveServer2. > - hs2_submitted_queries - is showing the number of queries currently > submitted to HS2 and being processed as well as the min/max/mean execution > times for the last 1028 measurements. > - hs2_compiling_queries - is showing the number of queries currently in the > compilation phase and the min/max/mean execution times for the last 1028 > measurements. > - hs2_executing_queries - is showing the number of queries currently in the > execution phase and the min/max/mean execution times for the last 1028 > measurements. > - hs2_failed_queries - is showing the total number of failed queries, as well > as the query failure rate exponentially weighted moving average for the last > 5/10/15 minutes > - hs2_suceeded_queries - is showing the total number of successful queries, > as well as the query success rate exponentially weighted moving average for > the last 5/10/15 minutes > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java > ba2267b29eaeb497e9582ca3ff3de4ad63bf8482 > common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java > 9b263d91321f141adf73e5421f3d659f80081471 > > common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java > c9d4087c5aeaf7f1ca0e9b5860b898b5766cd911 > > common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java > 9525b452eada8ab6a2dd56895f9c9d4eb50db894 > common/src/java/org/apache/hadoop/hive/ql/log/PerfLogger.java > 7658f1c3e0ae7112d10aaf195197ed7e0d318351 > common/src/test/org/apache/hadoop/hive/common/metrics/MetricsTestUtils.java > 46676589e6656d0f13f1931bfe67a63dd1920042 > > common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java > 6ee6245c1212c06c2ca9cc7a795f288c3928d675 > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 530d2f486de9ee92fc627404a3ae1ad086549e19 > ql/src/java/org/apache/hadoop/hive/ql/Driver.java > dd5543445ecb7387fad0cb861b9eb720edd06250 > ql/src/java/org/apache/hadoop/hive/ql/hooks/MetricsQueryLifeTimeHook.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/hooks/TestMetricsQueryLifeTimeHook.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 36c6f938316b65cc5444bb4d425066d26847ed70 > service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java > abdf8cd2f7abe2356e86b0eb19cf13311240deb8 > > service/src/test/org/apache/hive/service/cli/operation/TestSQLOperationMetrics.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52684/diff/ > > > Testing > ------- > > Unit tests added for existing and new metrics in the SQLOperation and > CodahaleMetrics classes. > Unit tests added for the new MetricsQueryLifeTimeHook class. > Manually tested the new metrics by executing queries in HS2. > > > Thanks, > > Barna Zsombor Klara > >
