Yongjun Zhang 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 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: Thu, 27 Dec 2018 04:19:35 +0000 Gerrit-HasComments: No
