Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 5: (5 comments) I'm ready to +1 after these issues and your testing http://gerrit.cloudera.org:8080/#/c/10060/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10060/5//COMMIT_MSG@24 PS5, Line 24: TODO: change terminology of "in_flight_query" to "submitted_queries" I'm ok with doing this as a follow-up if you prefer. http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.h@151 PS5, Line 151: /// that these updates reach the coordinator object after Exec() has been called on it. Are these comments stale? It seems like these functions are just straightforward wrappers now. http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/client-request-state.cc@735 PS5, Line 735: || operation_state_ < operation_state) Need braces for multi-line if. Can we be stricter about state machine for the PENDING case? I.e. check that it only transitions into the states that we expect to succeed PENDING? http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10060/5/be/src/service/impala-server.cc@673 PS5, Line 673: "Query id $0 has not started any backends yet.", PrintId(query_id)); Does this interact with live_summary=1 in the shell at all? http://gerrit.cloudera.org:8080/#/c/10060/5/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/10060/5/tests/hs2/test_hs2.py@456 PS5, Line 456: # Wait for query to start running. Should we try fetching the summary before admission control finishes? To exercise the error path. -- To view, visit http://gerrit.cloudera.org:8080/10060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I989cf5b259afb8f5bc5c35590c94961c81ce88bf Gerrit-Change-Number: 10060 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 01 May 2018 22:56:28 +0000 Gerrit-HasComments: Yes
