Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13786 )
Change subject: IMPALA-7322: Add storage wait time to profile ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG@18 PS2, Line 18: Testing: Ran queries that can trigger all of, none of or : some of the related tables loading. : Check query profile for each query. : Check catalog metrics for each table. : Add unit tests to test_observability.py : Ran all core tests. nit: if these are a list of testing activities done, i generally like to format it as follows Testing: * Ran queries that can trigger all of, none of or some of the related tables loading. * Check query profile for each query. * Check catalog metrics for each table. * dd unit tests to test_observability.py. * Ran all core tests. http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG@27 PS2, Line 27: After ran a hbase query (Metadata load finished is divided into several lines nit: line too long http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG@28 PS2, Line 28: because of limitation of commit message): Query Compilation: 4s401ms nit: the profile snippet should be on a newline http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG@35 PS2, Line 35: Profile for Catalog V2: would be good to explain how storage-load-time is represented in the v1 profile vs the v2 profile http://gerrit.cloudera.org:8080/#/c/13786/2//COMMIT_MSG@46 PS2, Line 46: storage-load-time nit: why is this formatted differently from everything else - e.g. would - CatalogFetch.Metadata.Storage.Time Be more consistent? I'm assuming "CatalogFetch.ColumnStats.Time" is how long it takes to load column stats? http://gerrit.cloudera.org:8080/#/c/13786/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/13786/2/common/thrift/CatalogObjects.thrift@496 PS2, Line 496: 15: optional i64 storage_metadata_load_time nit: change to storage_metadata_load_time_ns so we know it is in nanoseconds http://gerrit.cloudera.org:8080/#/c/13786/2/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/13786/2/tests/query_test/test_observability.py@23 PS2, Line 23: SkipIfCatalogV2 is this needed? http://gerrit.cloudera.org:8080/#/c/13786/2/tests/query_test/test_observability.py@684 PS2, Line 684: def test_query_profile_storage_load_time(self): nit: might be cleaner to split this into two tests, one for kudu and one for hbase -- To view, visit http://gerrit.cloudera.org:8080/13786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7447f8c8e7e50eb71d18643859d2e3de865368d2 Gerrit-Change-Number: 13786 Gerrit-PatchSet: 2 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Comment-Date: Fri, 06 Sep 2019 16:51:06 +0000 Gerrit-HasComments: Yes
