[Impala-ASF-CR] IMPALA-6742: Profiles of running queries should include execution summary.

["Joe McDonnell (Code Review)" <ge...@cloudera.org>]

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 4:

> > 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.
 > Hi Joe,
 > 
 > Thanks a lot for the review and comments and sorry for getting back
 > to this late. Very good suggestions!
 > 
 > I just uploaded a new rev to address it by adding a new unit test
 > per your comment #2. I also did the manual test of adding a sleep
 > call between L516-L517, I did not observe behavior difference.
 > Would you please help taking a look?
 > 
 > Thanks.

To be clear, for the test of the sleep between L516-L517, you fetched the profile while it was sleeping between those two lines?

Either way, the code change looks good. Please fix the flake8 comment, rebase and rerun tests, then I will +2.


-- 
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: 4
Gerrit-Owner: Yongjun Zhang <yj...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Yongjun Zhang <yj...@apache.org>
Gerrit-Comment-Date: Fri, 04 Jan 2019 20:51:54 +0000
Gerrit-HasComments: No