Quanlong Huang 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 3: (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? Thanks for the quick review! Currently, only the timestamp of table/view are used in impalad so only those are applied. For other types users can check them in the /catalog WebUI of catalogd. We can apply them in impalad in the future e.g. if we find a place to show them in profiles. Updated the commit message about this. 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" func Yes, we need it in ImpaladCatalog. When impalad applies the update from catalogd, it sets the catalog version and mTimeMs_ is updated as well. We need to reset the mTimeMs_ to the actually timestamp sent from catalogd: https://gerrit.cloudera.org/c/21782/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java#491 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: > Nit: these two functions could be overridden in the parent IcebergDeleteTab Good point! Done. 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: > Nit: these two functions could be overridden in the parent IcebergDeleteTab Done 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, a They are currently not used in impalad yet, e.g. not shown in the query profiles. I think it's OK to limit the scope of this change. 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, a Same as above, just set it for tables to be simple. 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: > Nit: add a blank line before this line and start the comment with // Add th Done http://gerrit.cloudera.org:8080/#/c/21782/2/fe/src/main/java/org/apache/impala/service/Frontend.java@2585 PS2, Line 2585: // Add the catalog versions and loaded timestamps. > Can this profile text be a formatted table like ExecSummary is? I hope it can be parsed by tools like Workload Management. Maybe the current form is easier for parsing. Or do we have a way to pretty print this only in the TEXT profile, and keep the THRIFT/JSON profile using this form? -- 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: 3 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 23:06:40 +0000 Gerrit-HasComments: Yes
