Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12940 )
Change subject: IMPALA-7322: Add storage wait time to profile ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/12940/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/12940/3/common/thrift/CatalogObjects.thrift@474 PS3, Line 474: Set iff this is a table needs storage access. I think this is vague. Better description? http://gerrit.cloudera.org:8080/#/c/12940/3/common/thrift/CatalogObjects.thrift@476 PS3, Line 476: storage_ld_time Try to use meaningful variable names whenever possible (for readability). say something like filesystem_metadata_load_time or something along those lines? http://gerrit.cloudera.org:8080/#/c/12940/3/common/thrift/CatalogObjects.thrift@476 PS3, Line 476: optional Why is it optional? http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@229 PS3, Line 229: strgTmNano meaningful variable names (for readability) http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@232 PS3, Line 232: ldTbl Better variable names http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@232 PS3, Line 232: loadedTbls_ How do you distinguish between tables loaded now vs tables already loaded. For ex: if 3 of 5 tables need loading (meaning 2 already cached), this loop seems to sum the metric for all the 5 tables. http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@233 PS3, Line 233: ldTbl instanceof Table This is kinda obvious, force a downcast? http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java: http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@105 PS3, Line 105: ctxStg better variable names http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@109 PS3, Line 109: hbaseTableName_ = Util.getHBaseTableName(getMetaStoreTable()); : // Warm up the connection and verify the table exists. : Util.getHBaseTable(hbaseTableName_).close(); : columnFamilies_ = null; : cols = Util.loadColumns(msTable_); : } finally { : storageLdTime_ = ctxStg.stop(); I don't think any of this calls into HBase http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1215 PS3, Line 1215: storageLdTm Better variable names http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1237 PS3, Line 1237: final Timer.Context ctxStg = I think the scopes for metrics are incorrect. I'd suggest to do this on top of https://gerrit.cloudera.org/#/c/12950/ which refactors the code nicely. http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/Table.java@115 PS3, Line 115: // Storage load time for the table Time spent in the source systems loading the fs metadata... (or something similar?) http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/Table.java@116 PS3, Line 116: storageLdTime_ better name? fileSystemMdLoadTime or something along those lines? http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/Table.java@193 PS3, Line 193: STORAGE_LOAD_DURATION_METRIC better name -- To view, visit http://gerrit.cloudera.org:8080/12940 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6dde7e394b7c1c396d835ef6aa0a55930c0a8660 Gerrit-Change-Number: 12940 Gerrit-PatchSet: 3 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Mon, 08 Apr 2019 23:16:19 +0000 Gerrit-HasComments: Yes
