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 4: (3 comments) Just a few more comments. Thanks! http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@70 PS4, Line 70: LOG.info("Thrift profile for query %s not yet available: %s", query_id, str(e)) This is really esoteric, but you can get python to print the exception by passing "exc_info=True" or using LOG.exception. An example: >>> import logging >>> logging.basicConfig(level=logging.INFO) >>> try: ... raise Exception("hey") ... except: ... logging.exception("x") ... logging.info("y", exc_info=True) ... ERROR:root:x Traceback (most recent call last): File "<stdin>", line 2, in <module> Exception: hey INFO:root:y Traceback (most recent call last): File "<stdin>", line 2, in <module> Exception: hey It doesn't really matter here, but it's a useful think to know. Feel free to change this without further review. http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@81 PS4, Line 81: return None I don't think this should return None. It's not expected that we can't deserialize this, right? So, we should fail the test. http://gerrit.cloudera.org:8080/#/c/8784/4/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/8784/4/tests/query_test/test_observability.py@155 PS4, Line 155: dbg_str = 'Debug thrift profile for query %s' + str(query_id) + ' not available in ' I don't think that %s is interpolated. -- 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: 4 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: Wed, 13 Dec 2017 21:18:38 +0000 Gerrit-HasComments: Yes
