Zoram Thanga 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 2: (9 comments) Thanks for the comments. Please have a look at patch set #3 http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@116 PS2, Line 116: def __test_query_profile_thrift_timestamps(self): > I wouldn't have a function named __x as well as a function named x. It's ju Removed __, and the other function below. http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@126 PS2, Line 126: service = ImpalaCluster().get_any_impalad().service > Why is this "any_impalad"? It really has to be the one that you executed th Is there a way to get this from the handle or something? Would love to use it. Changed to get_first_impalad() for now. http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@132 PS2, Line 132: tree = service.get_thrift_profile(query_id) > Where is get_thrift_profile() defined? This is defined in common/impala_service.py, as part of this patch. I figured it could be useful more generally. http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@134 PS2, Line 134: start_time = tree.nodes[1].info_strings["Start Time"] > Add a comment about why it's nodes[1] here. Done http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@136 PS2, Line 136: start_time_sub_sec_str = start_time.split('.')[-1] > Add a comment here about what start_time looks like, so it's clear what the Done http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@142 PS2, Line 142: assert len(end_time_sub_sec_str) == 9 > Add ", end_time" here, so that we can see the failed string if it ever fail Done http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@147 PS2, Line 147: # This could happen due to heavy system load. The test is then inconclusive. > This is a controlled environment--our tests. Let's be assertive and do the Changed to assert statement http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@152 PS2, Line 152: print dbg_str > Prefer logging to printing. We do both, but "import logging; logging.info(. Done http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@155 PS2, Line 155: def test_query_profile_thrift_timestamps(self): : if not self.__test_query_profile_thrift_timestamps(): : dbg_str = 'Debug thrift profile not available in ' + str(MAX_RETRIES) + ' seconds?' : dbg_str += ' Retrying the test' : assert True, self.__test_query_profile_thrift_timestamps() > Do we really need to run it twice? Why isn't it passing? I'll bet my hat on Removed this one. Instead asserting that the actual test above is successful. -- 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: 2 Gerrit-Owner: Zoram Thanga <zo...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Fri, 08 Dec 2017 07:03:36 +0000 Gerrit-HasComments: Yes
