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 12: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/12940/12/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/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@509 PS12, Line 509: nit: mention the unit too.. ns ms...? http://gerrit.cloudera.org:8080/#/c/12940/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@959 PS12, Line 959: try { : storageMetadataLoadTime_ += updateMdFromHmsTable(msTbl); : if (msTbl.getPartitionKeysSize() == 0) { : if (loadParitionFileMetadata) { : storageMetadataLoadTime_ += updateUnpartitionedTableFileMd(); : } : } else { : storageMetadataLoadTime_ += updatePartitionsFromHms( : client, partitionsToUpdate, loadParitionFileMetadata); : } : } finally { : storageLdTimer.update(storageMetadataLoadTime_, TimeUnit.NANOSECONDS); : } : LOG.info("Incrementally loaded table metadata for: " + getFullName()); : } else { : LOG.info("Fetching partition metadata from the Metastore: " + getFullName()); : // Load all partitions from Hive Metastore, including file metadata. : List<org.apache.hadoop.hive.metastore.api.Partition> msPartitions = : MetaStoreUtil.fetchAllPartitions( : client, db_.getName(), name_, NUM_PARTITION_FETCH_RETRIES); : LOG.info("Fetched partition metadata from the Metastore: " + getFullName()); : try { : storageMetadataLoadTime_ = loadAllPartitions(msPartitions, msTbl); : } finally { : I think you could use a single try finally for storageLdTimer.update(storageMetadataLoadTime_, TimeUnit.NANOSECONDS); Avoids unnecessary nesting. http://gerrit.cloudera.org:8080/#/c/12940/11/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/12940/11/tests/query_test/test_observability.py@572 PS11, Line 572: end_time = tree.nodes[1].info_strings["End Time"] : assert len(end_time) == 0, end_time : # Save the last operation to be able to retrieve the profile after closing the query : last_op = handle.get_handle()._last_operation : self.hs2_client.close_query(handle) : tree = last_op.get_profile(TRuntimeProfileFormat.THRIFT) : end_time = tree.nodes[1].info_strings["End Time"] : assert end_time is not None : : def test_query_profile_contains_number_of_fragment_instance(self): : """Test that the expected section for number of fragment instance in : a query profile.""" : event_regexes = [r'Per Host Number of Fragment Instances'] : query = "select count (*) from functional.alltypes" : runtime_profile = self.execute_query(query).runtime_profile : self.__verify_profile_event_sequence(event_regexes, runtime_profile) : : def test_query_profile_storage_load_time_filesystem(self): > The test_query_profile_storage_load_time2 only tests kudu and hbase. For hb Ah sorry, I missed the S3 part, my bad. -- 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: 12 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: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Mon, 24 Jun 2019 18:29:34 +0000 Gerrit-HasComments: Yes
