----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43008/#review117098 -----------------------------------------------------------
This is awesome! Assume this works for all execution engines? Left some comments. ql/src/java/org/apache/hadoop/hive/ql/Driver.java (line 526) <https://reviews.apache.org/r/43008/#comment178163> would it be useful to add a helper function "isWebUIEnabled()" vs checking if the port != 0? ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 46) <https://reviews.apache.org/r/43008/#comment178168> should this be private? comment on what the key/values are and maybe rename to taskIdToTaskInfo? ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 49) <https://reviews.apache.org/r/43008/#comment178165> Comment that this is set once the task completes. ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 69) <https://reviews.apache.org/r/43008/#comment178164> I would expect updateTask() to take a Task object. Is this ever called? ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 93) <https://reviews.apache.org/r/43008/#comment178171> Does this call (and a few other the others) need to be synchronized? Seems like their vals are all set once in the ctor. ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 126) <https://reviews.apache.org/r/43008/#comment178178> newTask -> addTask() ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 130) <https://reviews.apache.org/r/43008/#comment178166> Maybe setTaskCompleted()? ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 132) <https://reviews.apache.org/r/43008/#comment178167> When would taskInfo be null? returnval wouldn't get set and the query would still show up as running. ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 141) <https://reviews.apache.org/r/43008/#comment178169> just make this method synchronized? ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 169) <https://reviews.apache.org/r/43008/#comment178180> for the methods that accept maps, comment on the expected key/value for the parameters. ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 189) <https://reviews.apache.org/r/43008/#comment178179> 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. ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java (line 219) <https://reviews.apache.org/r/43008/#comment178170> private static. "Unknown" -> "UNKNOWN" service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java (line 26) <https://reviews.apache.org/r/43008/#comment178172> Comment on thread safety. service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java (line 53) <https://reviews.apache.org/r/43008/#comment178175> Is this called? Looks like a dupe of close() service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java (line 69) <https://reviews.apache.org/r/43008/#comment178173> Do the calls that return state copied in the ctor need to be synchronized? service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java (line 90) <https://reviews.apache.org/r/43008/#comment178174> check that state != null? service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java (line 98) <https://reviews.apache.org/r/43008/#comment178176> setClosed()? service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplayCache.java (line 26) <https://reviews.apache.org/r/43008/#comment178177> Does this need to extend LinkedHashMap or can you just create an instance of one? Are removeEldestEntry and capacity used? - Lenni Kuff On Jan. 30, 2016, 2:53 a.m., Szehon Ho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43008/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2016, 2:53 a.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/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 > >
