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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Yongjun Zhang <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 20:51:54 +0000 Gerrit-HasComments: No
