Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11591 )
Change subject: IMPALA-6742: Profiles of running queries should include execution summary. ...................................................................... Patch Set 3: I would like there to be a test that breaks if this change is not there (or somehow this breaks later). query_test/test_observability.py is the right place to add this test. Here are a couple ways to do this: 1. Use a sleep in the query. See query_test/test_rows_availability.py: "select * from functional.alltypestiny where month = 1 and bool_col = sleep(1000);" will take two seconds. Read that test and understand why the sleep works. 2. Execute a query that returns rows, and get the profile before fetching any of the rows. If we haven't called fetch, then the client has not called ImpalaServer::UnregisterQuery(). Of these, I think I prefer #2. See test_exec_summary() for an example. Unrelated to your change, I would like you to hand test your change by putting a sleep between these two lines: https://github.com/apache/impala/blob/master/be/src/service/client-request-state.cc#L516-L517 Then fetch the profile and verify it doesn't crash. I have a theory that this might not work. -- To view, visit http://gerrit.cloudera.org:8080/11591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc7f714c9427d4b26d4e78cf27ceca2b0b336699 Gerrit-Change-Number: 11591 Gerrit-PatchSet: 3 Gerrit-Owner: Yongjun Zhang <yjzhan...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Yongjun Zhang <yjzhan...@apache.org> Gerrit-Comment-Date: Wed, 14 Nov 2018 20:57:54 +0000 Gerrit-HasComments: No
