Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22653 )
Change subject: IMPALA-13884: Add more details in metadata loading logs ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/22653/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22653/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-13884: Add more details in metadata loading logs > Any concerns with breaking backwards compatibility on log messages? Are th I think we don't guarantee logs are in the same format between upgrades. The best effort is just adding more info in the logs so simple tools like counting Exceptions can still work. But that could still break tools that use regex patterns to match some long strings. http://gerrit.cloudera.org:8080/#/c/22653/5//COMMIT_MSG@9 PS5, Line 9: Adds the target of the event in HMS event processing logs. For > Are there any events that could also have the associated query id logged wi The HMS notification events are generated by HMS so they don't have Impala query ids. They might come from external systems like Hive or Spark. And currently there is no way for Impala to inject the query id when HMS generating the events. BTW, if the event is generating by Impala statements, it will be ignored by self-event evaluation. Maybe not much info to log. http://gerrit.cloudera.org:8080/#/c/22653/7/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/22653/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@636 PS7, Line 636: // Skip the first method which is always java.lang.Thread.getStackTrace(). > This approach is certainly the easiest, but I am concerned having a Throwab Done. Ideally we should just log the reason/goal to acquire the lock similar to how we log the reason for metadata loading. But there are too many call sites to change. http://gerrit.cloudera.org:8080/#/c/22653/7/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/22653/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6374 PS7, Line 6374: String annotation = String.format("Recovering %d partitions for %s", > Does the '%d' here need to be replaced with '%s'? String.format() still uses different kinds of place holder for different data types. Just the Preconditions methods need the change. http://gerrit.cloudera.org:8080/#/c/22653/7/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/22653/7/fe/src/main/java/org/apache/impala/util/AcidUtils.java@137 PS7, Line 137: public static boolean isTransactionalTable(Map<String, String> props) { > Is this function still needed or can it be deleted? It's still used in some places where we don't have the FeTable object, e.g. when checking the msTable from HMS: https://github.com/apache/impala/blob/8101b0784ccd1b87b85c1a055e92866614eb36a8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L4484 -- To view, visit http://gerrit.cloudera.org:8080/22653 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I204d5922e055fd8501b5573e3b913f8874d891d6 Gerrit-Change-Number: 22653 Gerrit-PatchSet: 9 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Thu, 27 Mar 2025 02:41:32 +0000 Gerrit-HasComments: Yes
