Jason Fehr 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 7: (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 there customers who have monitoring solutions looking for any of the log lines that were modified? 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 with them? For example, the AlterPartitionEvent would include the id of the "alter partition ..." DDL. Having query the query id appear in the catalogd logs is very helpful when tracing the lifecycle of a query across daemons. 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: new Throwable("Stack trace of the caller")); This approach is certainly the easiest, but I am concerned having a Throwable in the logs that is not really an error will result in confusion and time spent digging into these throwables. Please consider using `Thread.currentThread().getStackTrace()` to log a non-exception stack trace instead. 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'? 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? -- 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: 7 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: Wed, 26 Mar 2025 16:04:41 +0000 Gerrit-HasComments: Yes
