Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20491 )
Change subject: IMPALA-12443: Add catalog timeline for all DDL profiles ...................................................................... Patch Set 12: (13 comments) Great work, Quanlong! Left a couple of comments, but looks really good! http://gerrit.cloudera.org:8080/#/c/20491/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20491/12//COMMIT_MSG@31 PS12, Line 31: unloaded reloaded? http://gerrit.cloudera.org:8080/#/c/20491/12/be/src/exec/catalog-op-executor.cc File be/src/exec/catalog-op-executor.cc: http://gerrit.cloudera.org:8080/#/c/20491/12/be/src/exec/catalog-op-executor.cc@125 PS12, Line 125: catalog_profile_ = make_unique<TRuntimeProfileNode>(exec_response_->profile); Would it make sense to do a sanity check that we didn't get an awful lot of timeline events, e.g. events should be less than 100? http://gerrit.cloudera.org:8080/#/c/20491/12/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/20491/12/be/src/service/client-request-state.cc@1596 PS12, Line 1596: catalog_timeline.name nit: could be 'timeline_name' http://gerrit.cloudera.org:8080/#/c/20491/12/be/src/service/client-request-state.cc@1773 PS12, Line 1773: if (catalog_op_executor_->catalog_profile() != nullptr) { : for (const TEventSequence& catalog_timeline : : catalog_op_executor_->catalog_profile()->event_sequences) { : summary_profile_->AddEventSequence(catalog_timeline.name, catalog_timeline); : } : } nit: same as L711-L716. Could be moved to a member function. http://gerrit.cloudera.org:8080/#/c/20491/12/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/20491/12/common/thrift/CatalogService.thrift@296 PS12, Line 296: DDL Maybe DDL/DML? E.g. this will also contain information about Iceberg DML operations. http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2014 PS12, Line 2014: Loaded TableMeta nit: I found the camelcase word a bit weird here, so maybe 'Loaded table metadata'? http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2312 PS12, Line 2312: versionLock_.readLock().lock(); : catalogTimeline.markEvent(GOT_CATALOG_VERSION_READ_LOCK); Would it make sense to create a method that encapsulates these two? http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2347 PS12, Line 2347: tbl.readLock().lock(); : catalogTimeline.markEvent(GOT_TABLE_READ_LOCK); Would it make sense to create a method that encapsulates these two? http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java: http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@115 PS12, Line 115: hbase nit: HBase? http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@324 PS12, Line 324: kudu nit: Kudu http://gerrit.cloudera.org:8080/#/c/20491/9/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/20491/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7109 PS9, Line 7109: // always responsible for aborting transactions when queries hit errors. > Sure. update.getIceberg_operation() returns a TIcebergOperationParam which Thank you! http://gerrit.cloudera.org:8080/#/c/20491/12/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/20491/12/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1514 PS12, Line 1514: catalogTimeline.markEvent("Updated table in Iceberg"); This might be misleading, as we don't update the table until we commit. So I think it's enough to only keep the "Committed Iceberg transaction". Also, https://gerrit.cloudera.org/#/c/20823/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java will get rid of 'needsTxn' anyway. http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/util/EventSequence.java File fe/src/main/java/org/apache/impala/util/EventSequence.java: http://gerrit.cloudera.org:8080/#/c/20491/12/fe/src/main/java/org/apache/impala/util/EventSequence.java@50 PS12, Line 50: new EventSequence("unused"); Not sure if it worth the effort, but we could inherit from EventSequence, e.g. creating class NoOpEventSequence, that would do nothing on markEvent(), etc., and we could return a static object here. -- To view, visit http://gerrit.cloudera.org:8080/20491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbceefaeb24c66eb1a064c449d6f56077ea347c5 Gerrit-Change-Number: 20491 Gerrit-PatchSet: 12 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zihao Ye <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 22 Dec 2023 19:05:38 +0000 Gerrit-HasComments: Yes
