Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8784 )
Change subject: IMPALA-6225: Part 2: Query profile date-time strings should have ns precision. ...................................................................... Patch Set 3: (3 comments) Thanks for the iteration. I think this is almost done. Just a few more comments. http://gerrit.cloudera.org:8080/#/c/8784/3/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/8784/3/tests/common/impala_service.py@64 PS3, Line 64: def get_thrift_profile(self, query_id, timeout=10, interval=1): please add pydoc. This will get used again, I'm sure. http://gerrit.cloudera.org:8080/#/c/8784/3/tests/common/impala_service.py@73 PS3, Line 73: LOG.info("Thrift profile for query %s not yet available.") Is there a more specific exception you can use? Alternately, put the try/except around line 68. I worry that zlib.decompress, and base64, and deserialize can all throw interesting exceptions that I wouldn't want to swallow. Also: should this be returning None? or re-throwing? Are you intentionally returning an empty thrift thingy? http://gerrit.cloudera.org:8080/#/c/8784/3/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/8784/3/tests/query_test/test_observability.py@125 PS3, Line 125: service = ImpalaCluster().get_first_impalad().service I think this maybe should be service = self.cluster.impalads[0].service i.e., you shouldn't be creating a new ImpalaCluster(), but rather using the onet hat your test has provided you. "git grep self.cluster.get" is more prominent than "git grep ImpalaCluster\(", though the latter certainly identifies several classes of this bug. 09:03:15 [130]pannier Impala [maven ?*] ~/src/Impala $git grep self.cluster.get bin/remote_data_load.py: services = dict((s.type, s) for s in self.cluster.get_all_services()) tests/common/failure_injector.py: self.cluster.get_impala_service().set_process_auto_restart_config(value=True) tests/common/failure_injector.py: # self.cluster.get_impala_service().restart() tests/common/failure_injector.py: num_impalad_procs = len(self.cluster.get_impala_service().get_all_impalad_processes()) tests/common/failure_injector.py: self.cluster.get_impala_service().get_all_impalad_processes()) tests/common/failure_injector.py: state_store = self.cluster.get_impala_service().get_state_store_process() tests/comparison/cluster.py: endpoint = self.cluster.get_hadoop_config("dfs.namenode.http-address", tests/comparison/cluster.py: port = self.cluster.get_hadoop_config("dfs.https.port", 20102) tests/comparison/cluster.py: self.cluster.get_hadoop_config("hive.metastore.warehouse.dir")).path tests/custom_cluster/test_insert_behaviour.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_insert_behaviour.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_query_concurrency.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_query_expiration.py: impalad = self.cluster.get_first_impalad() tests/custom_cluster/test_query_expiration.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_query_expiration.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_s3a_access.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_scratch_disk.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_scratch_disk.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_scratch_disk.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_scratch_disk.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_scratch_disk.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_seq_file_filtering.py: impalad = self.cluster.get_any_impalad() tests/custom_cluster/test_session_expiration.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_catalog_hms_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_catalog_hms_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_catalog_hms_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_process_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_process_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_process_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_process_failures.py: worker_impalad = self.cluster.get_different_impalad(impalad) tests/experiments/test_process_failures.py: impalad = self.cluster.get_any_impalad() tests/experiments/test_process_failures.py: impalad = self.cluster.get_any_impalad() -- To view, visit http://gerrit.cloudera.org:8080/8784 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109 Gerrit-Change-Number: 8784 Gerrit-PatchSet: 3 Gerrit-Owner: Zoram Thanga <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Zoram Thanga <[email protected]> Gerrit-Comment-Date: Fri, 08 Dec 2017 22:31:23 +0000 Gerrit-HasComments: Yes
