hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szehon Ho <sze...@cloudera.com>
Subject Re: Review Request 43008: HIVE-12952 : Show query sub-pages on webui
Date Mon, 01 Feb 2016 20:08:24 GMT


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 528
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227021#file1227021line528>
> >
> >     would it be useful to add a helper function "isWebUIEnabled()" vs checking if
the port != 0?

Made new method in HiveConf.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 46
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line46>
> >
> >     should this be private? comment on what the key/values are and maybe rename
to taskIdToTaskInfo?

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 49
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line49>
> >
> >     Comment that this is set once the task completes.

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 69
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line69>
> >
> >     I would expect updateTask() to take a Task object.
> >     
> >     Is this ever called?

it was un-used, removed the method.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 93
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line93>
> >
> >     Does this call (and a few other the others) need to be synchronized? Seems like
their vals are all set once in the ctor.

Nope, final should be enough as you mentioned.  Removed synchronized.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 126
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line126>
> >
> >     newTask -> addTask()

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 130
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line130>
> >
> >     Maybe setTaskCompleted()?

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 141
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line141>
> >
> >     just make this method synchronized?

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 189
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line189>
> >
> >     clarify what "times" means. Consider having a single: setExecTimes(startTimes,
endTimes) or something if you think both should always be set at the same time.

Added comment.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 169
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line169>
> >
> >     for the methods that accept maps, comment on the expected key/value for the
parameters.

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java, line 219
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227022#file1227022line219>
> >
> >     private static.
> >     "Unknown" -> "UNKNOWN"

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java,
line 26
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227029#file1227029line26>
> >
> >     Comment on thread safety.

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java,
line 53
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227029#file1227029line53>
> >
> >     Is this called? Looks like a dupe of close()

Removed un-used method.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java,
line 69
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227029#file1227029line69>
> >
> >     Do the calls that return state copied in the ctor need to be synchronized?

Nope, added final and removed synchronized.


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java,
line 98
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227029#file1227029line98>
> >
> >     setClosed()?

Done


> On Jan. 30, 2016, 6:28 a.m., Lenni Kuff wrote:
> > service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplayCache.java,
line 26
> > <https://reviews.apache.org/r/43008/diff/1/?file=1227030#file1227030line26>
> >
> >     Does this need to extend LinkedHashMap or can you just create an instance of
one? Are removeEldestEntry and capacity used?

Actually this is the bizarre way to implement LRU cache in java:

https://docs.oracle.com/javase/8/docs/api/java/util/LinkedHashMap.html#removeEldestEntry-java.util.Map.Entry-


- Szehon


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


On Feb. 1, 2016, 8:05 p.m., Szehon Ho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43008/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2016, 8:05 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-12952
>     https://issues.apache.org/jira/browse/HIVE-12952
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> This patch shows a query sub-page on WebUI, with detailed information of query on differnt
tabs:
> 
> 1.  Tabl- Base Info, ie user, query string, query id, begin time, end time, execution
engine, error (if any)
> 2.  Tab2- Query Plan
> 3.  Tab3- Stages (MR jobs), their progress and info
> 4.  Tab4- Call trace info captured from HMSClient and PerfLogger.
> 
> Implementation notes:
> The UI design choices are inspired from Impala, and HBase.  This, like HBase webui, uses
Jamon, which is a superset of JSP and makes dynamic content a lot easier.  As such, brought
in jamon dependency and also js bootstrap libraries to support the dynamic tabs.
> 
> On Hive side, refactored webui query logic into following classes:  SQLoperationDisplay
(info captured from SQLOperation), QueryDisplay (info captured from Driver).
> 
> 
> TODO:
> 1. Hard to get more MR job information for the stages including a job-tracking url, due
to MR JobSubmission being a separate process, need to think about it.  Same for Spark/tez.
> 2. The explain plan might be a bit bulky and consume a bit of memory (though can tune
with "hive.server2.webui.max.historic.queries").  Perhaps in future we can spill to local
disk, and stream from there.  This might also help with (1), if we don't want to implement
inter-process communciations.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java ff376a8 
>   common/src/java/org/apache/hadoop/hive/ql/log/PerfLogger.java d4194cf 
>   common/src/java/org/apache/hive/http/HttpServer.java 9e23b11 
>   pom.xml 802d3d4 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 4c89812 
>   ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 0bab769 
>   service/pom.xml b2e3a84 
>   service/src/jamon/org/apache/hive/tmpl/QueryProfileTmpl.jamon PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 0c263cf 
>   service/src/java/org/apache/hive/service/cli/operation/OperationManager.java f1ce6f6

>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 01b1d3d 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java PRE-CREATION

>   service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplayCache.java
PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperationInfo.java 179f6dd

>   service/src/java/org/apache/hive/service/server/HiveServer2.java 958458f 
>   service/src/java/org/apache/hive/service/servlet/QueryProfileServlet.java PRE-CREATION

>   service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp a0b5d2e 
>   service/src/resources/hive-webapps/static/js/bootstrap.js PRE-CREATION 
>   service/src/resources/hive-webapps/static/js/bootstrap.min.js PRE-CREATION 
>   service/src/resources/hive-webapps/static/js/jquery.min.js PRE-CREATION 
>   service/src/resources/hive-webapps/static/js/tab.js PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43008/diff/
> 
> 
> Testing
> -------
> 
> Manual testing.  Can add some unit tests in follow-up.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message