Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23811 )
Change subject: IMPALA-14641: Add detailed operation page for Catalogd operations ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/23811/3/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/23811/3/be/src/catalog/catalog-server.cc@753 PS3, Line 753: { this->OperationDetailUrlCallback(args, doc); }, false); nit: move the opening brace above to be consistency with L749. http://gerrit.cloudera.org:8080/#/c/23811/3/be/src/catalog/catalog-server.cc@1603 PS3, Line 1603: if (PrintId(record.query_id) == query_id_str) { We need to check the thread id as well. Just checking the query id is not enough. A statement can have multiple catalog requests. E.g. CreateTableAsSelect statement first send a execDdl request to create the table, then send an updateCatalog request to finalize the insert. http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java File fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java: http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java@228 PS3, Line 228: EventSequence eventSeq = nit: Just invoke remove() and don't need to declare this unused variable. http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java@249 PS3, Line 249: byte[] bytes = serializer.serialize(response); It's a waste to serialize the response just for getting the size. Note that response could be super large (GB). It'd be better to set it in JniCatalog before returning the response. http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7643 PS3, Line 7643: // EventSequence is now passed in from the caller nit: remove this comment since it just explains the current change http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@319 PS3, Line 319: CatalogOperationTracker.INSTANCE.setRequestSize(thriftDdlExecReq.length); I think a better way to avoid using ThreadLocal is creating the objects here: * Create the EventSequence object in JniCatalog and pass it into execDdlRequest(), execResetMetadata() and updateCatalog(). * Add an update method to CatalogOperationTracker to add or update the record. We already have the request and thread id here so it's doable to target the RpcKey. * Use this update method in increment() to update other parts like catalog_op_name, target_name, etc. * Use this update method before returning the response here to set the response size. What do you think? http://gerrit.cloudera.org:8080/#/c/23811/3/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/23811/3/tests/webserver/test_web_pages.py@1092 PS3, Line 1092: self.execute_query("create table {0}.test_detail (id int)".format(unique_database)) Let's use a CreateTableAsSelect statement to have multiple catalog operations. http://gerrit.cloudera.org:8080/#/c/23811/3/tests/webserver/test_web_pages.py@1201 PS3, Line 1201: import time nit: move this to the import section of the file http://gerrit.cloudera.org:8080/#/c/23811/3/www/operation_detail.tmpl File www/operation_detail.tmpl: http://gerrit.cloudera.org:8080/#/c/23811/3/www/operation_detail.tmpl@98 PS3, Line 98: var detailsStr = "{{details}}"; This doesn't work if the details string is multi-lines. E.g. ", error=ImpalaRuntimeException: Error creating Kudu table 'impala::default.my_kudu_tbl' CAUSED BY: NonRecoverableException: not enough live tablet servers to create a table with the requested replication factor 3; 0 tablet servers are alive" I see an error of "Uncaught SyntaxError: Invalid or unexpected token" in the browser console. http://gerrit.cloudera.org:8080/#/c/23811/3/www/operation_detail.tmpl@133 PS3, Line 133: var timelineData = {{{timeline}}}; : : // Handle Thrift JSON format: {"1": {"str": "name"}, "2": {"lst": [...]}, "3": {"lst": [...]}} : // The lst format is: ["type", count, value1, value2, ...] : var timelineName = timelineData["1"] && timelineData["1"].str ? timelineData["1"].str : "Timeline"; : var timestamps = []; : var labels = []; : : if (timelineData["2"] && timelineData["2"].lst) { : timestamps = timelineData["2"].lst.slice(2); // Skip type and count : } : if (timelineData["3"] && timelineData["3"].lst) { : labels = timelineData["3"].lst.slice(2); // Skip type and count : } : : if (timestamps.length > 0 && labels.length > 0) { : var output = []; : var startTime = timestamps[0]; : var totalTimeNs = timestamps[timestamps.length - 1] - startTime; : : // Format time in human-readable format (like query profiles) : function formatTime(ns) { : var ms = ns / 1000000; : var s = Math.floor(ms / 1000); : ms = ms % 1000; : var us = (ns % 1000000) / 1000; : : if (s > 0) { : return s + "s" + Math.floor(ms) + "ms"; : } else if (ms >= 1) { : return ms.toFixed(3) + "ms"; : } else { : return us.toFixed(3) + "us"; : } : } : : // Add title with total time : output.push(timelineName + ": " + formatTime(totalTimeNs)); : : // Add each event with absolute and delta times : for (var i = 0; i < Math.min(timestamps.length, labels.length); i++) { : var absoluteTime = timestamps[i] - startTime; : var deltaTime = i > 0 ? timestamps[i] - timestamps[i-1] : 0; : : var line = " - " + labels[i] + ": " + : formatTime(absoluteTime) + " (" + formatTime(deltaTime) + ")"; : output.push(line); : } : : $('#timeline-display').text(output.join('\n')); : } else { : $('#timeline-display').text('No timeline events available'); : } : } catch (e) { : $('#timeline-display').text('Error parsing timeline: ' + e.message); : } It'd be better to use the existing logic in C++ to pretty-print the timeline: https://github.com/apache/impala/blob/3e29f8aaa8f1a02630505866de45a918918aa216/be/src/util/runtime-profile.cc#L1574-L1586 So the string can be consistent with profiles in coordinator side. -- To view, visit http://gerrit.cloudera.org:8080/23811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib80ce3b5873672065b04b00a21d3419a1db0969c Gerrit-Change-Number: 23811 Gerrit-PatchSet: 3 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 20 Jan 2026 08:10:27 +0000 Gerrit-HasComments: Yes
