Yongzhi Chen 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) Submit patch 3 to address the code review 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 fo Done 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 Done 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 Done 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 pro Done 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 I will change to CatalogFetch.StorageLoad.Time which consistent with V1 and has three levels. 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 nanosecond Done 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? Done 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 fo I think they can put together: They are similar and do not belong to filesystem loading. -- 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 <yc...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Mon, 09 Sep 2019 16:15:30 +0000 Gerrit-HasComments: Yes
