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 4: (10 comments) Thanks Quanlong for the review! 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: false); > nit: move the opening brace above to be consistency with L749. Done http://gerrit.cloudera.org:8080/#/c/23811/3/be/src/catalog/catalog-server.cc@1603 PS3, Line 1603: // Thrift JSON list format: [type, count, val1, val2, ...] > We need to check the thread id as well. Just checking the query id is not e Done 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: System.currentTimeM > nit: Just invoke remove() and don't need to declare this unused variable. Done http://gerrit.cloudera.org:8080/#/c/23811/3/fe/src/main/java/org/apache/impala/catalog/monitor/CatalogOperationTracker.java@249 PS3, Line 249: record.setFinish_time_ms(System.currentTimeMillis()); > It's a waste to serialize the response just for getting the size. Note that Done 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: catalog_.getLock().writeLock().unlock(); > nit: remove this comment since it just explains the current change 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: * Gets the current catalog version. > I think a better way to avoid using ThreadLocal is creating the objects her Thanks for the detailed breakdown of the steps. However, I just wanted to ask whats the down side of using ThreadLocal? 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: """Test to check that the /healthz endpoint returns 200 OK.""" > Let's use a CreateTableAsSelect statement to have multiple catalog operatio Done http://gerrit.cloudera.org:8080/#/c/23811/3/tests/webserver/test_web_pages.py@1201 PS3, Line 1201: > nit: move this to the import section of the file 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@98 PS3, Line 98: <script> > This doesn't work if the details string is multi-lines. E.g. Ive modified this part now, covering all possible cases. 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 }} : : : : : : : : : : : : : : : : : : : : : > It'd be better to use the existing logic in C++ to pretty-print the timelin 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: 4 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: Sat, 24 Jan 2026 10:06:30 +0000 Gerrit-HasComments: Yes
