Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21782 )
Change subject: IMPALA-12876: Add catalogVersion and loaded timestamp in query profiles ...................................................................... Patch Set 2: (7 comments) The Catalog.java file sets the last modified time on db, table, view, function, data source, and hdfs cache pool. However, only the table and view are actually reported in the profile. Do we need to set last modified time on the other object types? http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@46 PS2, Line 46: public void setLastLoadedTimeMs(long timeMs) { Is this setter needed since mTimeMs_ is set in the "setCatalogVersion" function? http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/IcebergEqualityDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergEqualityDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/IcebergEqualityDeleteTable.java@70 PS2, Line 70: public long getCatalogVersion() { return 0; } Nit: these two functions could be overridden in the parent IcebergDeleteTable. http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/IcebergPositionDeleteTable.java@66 PS2, Line 66: public long getCatalogVersion() { return 0; } Nit: these two functions could be overridden in the parent IcebergDeleteTable. http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@346 PS2, Line 346: catalogObject.getCatalog_version(), catalogObject.getLast_modified_time_ms()); Does last modified time also need to be set on db, function, data source, and hdfs cache pool like it is set on Catalog.getTCatalogObject? http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@807 PS2, Line 807: resp.object_loaded_time_ms, new SqlConstraints(primaryKeys, foreignKeys), Does last modified time also need to be set on db, function, data source, and hdfs cache pool? http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/service/Frontend.java@2584 PS2, Line 2584: // Also adds the catalog versions and loaded timestamps. Nit: add a blank line before this line and start the comment with // Add the catalog... http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/service/Frontend.java@2585 PS2, Line 2585: FrontendProfile.getCurrent().addInfoString("Table Versions", Can this profile text be a formatted table like ExecSummary is? -- To view, visit http://gerrit.cloudera.org:8080/21782 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94b2fd59ed5aca664d6db4448c61ad21a88a4f98 Gerrit-Change-Number: 21782 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 11 Sep 2024 15:37:49 +0000 Gerrit-HasComments: Yes
