Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 12: (5 comments) Looks nice. Please go ahead and rebase when you have a chance. Tim may want to take a final look at this point. Do we have sufficient test coverage for the various webserver/RPC handlers that can happen concurrently with admission control (previously, less cases were possible since the query handle wasn't returned until after admission control)? http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/scheduling/admission-controller.h@212 PS9, Line 212: us AdmitQuery(QuerySchedule* schedule, > I agree, but its kinda hard to think of a good name since the patch is litt Works for me. Or leaving as-is also okay. http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/client-request-state.h@319 PS12, Line 319: async_ the async part seems redundant with "thread" to me, and inconsistent with our usual naming (e.g. wait_thread_ in this class is the thing that calls Wait(). Another example: build_thread_ is the thing that does the join build asynchronously). But if you feel it's better, then leave it. http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@176 PS9, Line 176: or* GetCoordinator( > Yea, I kinda struggled with naming this as well. Maybe: coord_exec_called_, then it's very literal so less chance for other interpretations? http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/10060/12/be/src/service/impala-http-handler.cc@776 PS12, Line 776: request_state->operation_state() != TOperationState::ERROR_STATE I guess that's for the case that the cancellation happens before the coordinator is set? stepping back, what is this case trying to catch? Should it really just be checking for non query and dml stmt types? Anyway, fine to leave this alone for now. http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/impala-server.h@429 PS11, Line 429: It represents the set of queries that : /// are either queued or have started executing. Used primarily to identif > on second thought, even "submitted_queries" is not a good name for this. "i Yeah, I'm fine with adding the bit about cancellation as long as we also state that it tracks queries to be closed with the session (like your new comment does). It was just misleading without also mentioning that. -- 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: 12 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, 22 May 2018 18:29:36 +0000 Gerrit-HasComments: Yes
