Arnab Karmakar 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 7: (15 comments) http://gerrit.cloudera.org:8080/#/c/23811/5/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/23811/5/be/src/catalog/catalog-server.cc@1579 PS5, Line 1579: CatalogOpListToJson(response.finished_catalog_operations, &finished_catalog_ops, > Processing the thrift JSON string is cumbersome. We can do things easier by Done http://gerrit.cloudera.org:8080/#/c/23811/5/be/src/catalog/catalog-server.cc@1693 PS5, Line 1693: } > I just realized (query_id, thread_id) can be duplicated for different RPCs Done http://gerrit.cloudera.org:8080/#/c/23811/5/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/23811/5/common/thrift/JniCatalog.thrift@916 PS5, Line 916: // Optional field for the timeline of execution in catalogd > We convert TEventSequence to JSON string in frontend and then convert it to Done http://gerrit.cloudera.org:8080/#/c/23811/5/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/5/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java@91 PS5, Line 91: if (!(o instanceof RpcKey)) return false; : RpcKey key = (RpcKey) o; : return queryId_.equals(key.queryId_) && threadId_ == key.threadId_; : } > I think we don't need this since all the usages on this are still in the sa Done http://gerrit.cloudera.org:8080/#/c/23811/5/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java@168 PS5, Line 168: String coordinator = "unknown"; > When will we update a finished record? We'd better avoid it. Thanks for pointing this, yes we should not update finished records. I refactored the flow to capture and set request_size_bytes and response_size_bytes 1. Request size: Passed down from JniCatalog -> CatalogOpExecutor -> CatalogOperationTracker.increment(), set when the record is created 2. Response size: Captured in JniCatalog after serialization, then passed to decrement() which calls updateRecord() to set it on the in-flight record before archiveRecord() moves it to finished I have made some drastic changes, after lots of thoughts as I couldn't think of any other solution. Ive tried to maintain backward compatibility, please feel free to suggest some other execution flow that'll be much cleaner and backward compatible. http://gerrit.cloudera.org:8080/#/c/23811/5/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/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@449 PS5, Line 449: > nit: add @VisibleForTesting annotaion since this is only used by tests now. Done http://gerrit.cloudera.org:8080/#/c/23811/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@451 PS5, Line 451: > nit: use CATALOG_TIMELINE_NAME Done 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: */ > TheadLocal variables are harder to maintain, e.g., lifecycle management nee Makes sense, thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/23811/5/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/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@352 PS5, Line 352: s.ddl_type, queryId, > The purpose of creating the catalogTimeline here is to add the TCatalogOpRe Done. Also removed the new map inFlightTimelines_, introduced a new static class InFlightOperation in CatalogOperationTracker for bundling the TCatalogOpRecord and EventSequence. http://gerrit.cloudera.org:8080/#/c/23811/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@354 PS5, Line 354: : : r > nit: simply it to () -> catalogOpExecutor_.execDdlRequest(params, catalogTi Done http://gerrit.cloudera.org:8080/#/c/23811/5/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/23811/5/tests/webserver/test_web_pages.py@1209 PS5, Line 1209: .format(unique_database)) > Get the query id from the result and verify it matches what we get from the Done http://gerrit.cloudera.org:8080/#/c/23811/5/tests/webserver/test_web_pages.py@1235 PS5, Line 1235: query_id = test_op["query_id"] > We just verify one operation. Please verify both of them. Done http://gerrit.cloudera.org:8080/#/c/23811/5/tests/webserver/test_web_pages.py@1259 PS5, Line 1259: data = jso > nit: These are new fields added by this commit. But not new fields after th Done http://gerrit.cloudera.org:8080/#/c/23811/5/tests/webserver/test_web_pages.py@1326 PS5, Line 1326: catalog_operations_page = requests.get("http://localhost:25020/operations?json";).text > Get the query id from 'handle' and verify it matches what we get from the W Done 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@133 PS3, Line 133: outDiv.appendChild(ul); : } else { : var div = document.createElement('div'); : div.style.wordBreak = 'break-word'; : div.textContent = detailsText; : outDiv.appendChild(div); : } : }); : </script> : </td> : </tr> : </table> : </div> : </div> : </div> : : {{?timeline}} : <div class="panel panel-info"> : <div class="card"> : <div class="card-header"> : <h5 class="card-title">Execution Timeline</h5> : </div> : <div class="card-body"> : <pre style="background-color: #f5f5f5; padding: 15px; border-radius: 5px; max-height: 500px; overflow-y: auto; font-family: monospace; white-space: pre;">{{timeline}}</pre> : </div> : </div> : </div> : {{/timeline}} : : <div style="margin-top: 20px;"> : <a href="/operations" class="btn btn-primary">Back to Operations List</a> : </div> : {{/error}} : : {{> www/common-footer.tmpl }} : : : : : : : : : : : : : : : : : : : : : > I see you wrote a C++ implementation of this. I think we don't need it. Rep Done -- 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: 7 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: Wed, 04 Feb 2026 16:47:37 +0000 Gerrit-HasComments: Yes
