Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18772 )
Change subject: IMPALA-11401,IMPALA-10794: Add logs and thread names for catalogd RPCs ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/18772/5/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/18772/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2633 PS5, Line 2633: > We have already created a ThreadNameAnnotator in JniCatalog.ExecDdl, right? Oops, this is part of a previous patch. I should have reverted them. http://gerrit.cloudera.org:8080/#/c/18772/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/18772/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@227 PS5, Line 227: Strin > Is there a reason for creating a ThreadAnnotator in some cases, but not doi I can try to add more. But some RPCs are easy to get their target. E.g. getCatalogDelta() is only called when statestore polls catalog updates. Some are dead codes, e.g. getDbs, getTableNames, getFunctions. We can remove them in a follow-up JIRA. http://gerrit.cloudera.org:8080/#/c/18772/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java@261 PS5, Line 261: JniUtil.logResponse(res.length, start, params, "exe > optional: logging the duration could be useful (or we do this somewhere els We calculate the duration in JniUtil.logResponse() and add warnings for slow response (>60s) there. We can add duration here to ease performance comparison. http://gerrit.cloudera.org:8080/#/c/18772/5/fe/src/main/java/org/apache/impala/util/CatalogOpUtil.java File fe/src/main/java/org/apache/impala/util/CatalogOpUtil.java: http://gerrit.cloudera.org:8080/#/c/18772/5/fe/src/main/java/org/apache/impala/util/CatalogOpUtil.java@133 PS5, Line 133: if (req.isSetDb_name()) { : if (req.is_refresh) cmd += "FUNCTIONS IN "; : cmd += "DATABASE " + req.getDb_name(); : } else if (req.isSetTable_name()) { > Is this the right order to check these? So if we are refreshing a table, wo This is consistent with the checks in execResetMetadata(). The name is misleading that "Table_name" here is not just the table name but a TTableName which consists of the db and table string: https://github.com/apache/impala/blob/29a26a536c004e4d16fdb56e1bb5a41605ae4e30/common/thrift/CatalogObjects.thrift#L171-L177 https://github.com/apache/impala/blob/29a26a536c004e4d16fdb56e1bb5a41605ae4e30/common/thrift/CatalogService.thrift#L278-L280 If the target is a database, the db_name is set: https://github.com/apache/impala/blob/29a26a536c004e4d16fdb56e1bb5a41605ae4e30/common/thrift/CatalogService.thrift#L286-L287 -- To view, visit http://gerrit.cloudera.org:8080/18772 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac7f2eda8b95643a3d3c3bef64ea71b67b20595a Gerrit-Change-Number: 18772 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Fri, 05 Aug 2022 08:44:05 +0000 Gerrit-HasComments: Yes
