Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 11: (30 comments) 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@44 PS9, Line 44: if > add "... or the caller wants to cancel" or something like that about cancel Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/scheduling/admission-controller.h@204 PS9, Line 204: Returns immediately if rejected > let's say what the return value and promise value are for each case, i.e. s Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/scheduling/admission-controller.h@206 PS9, Line 206: ANCELLED > i.e. explain status in this case. Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/scheduling/admission-controller.h@212 PS9, Line 212: Promise<AdmissionOutcome, PromiseMode::MULTIPLE_PRODUCER> > could create a typedef for that since it appears in several places and is k I agree, but its kinda hard to think of a good name since the patch is littered with permutations of {Admission, outcome, admit}, "AdmissionPromise" perhaps? http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/scheduling/admission-controller.cc@581 PS11, Line 581: result is set or the query is cancelled or it : // times out. > the "result is set" also happens in the cancel case. To clarify, how about Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/scheduling/admission-controller.cc@590 PS11, Line 590: SleepIfSetInDebugOptions(schedule->query_options(), SLEEP_AFTER_ADMISSION_OUTCOME_MS); > what's interesting about this time interval? Is it to give the dequeue loop Yes, this enables the dequeue thread to wake and get hold of the lock before this does. http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/scheduling/admission-controller.cc@607 PS11, Line 607: stats->Dequeue(*schedule, true); > nit: it was a bit hard to see that this block is really the same as the els Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@70 PS11, Line 70: /// the query to the Admission controller for asynchronous admission control. > is there some post condition around the operation state we can document? li yes, when this returns the operation state is either RUNNING_STATE or PENDING_STATE. Added this to the comment. http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@131 PS11, Line 131: and sets the : /// query status to CANCELLE > I thought you decided to hold off on that change. sorry, forgot to revert this comment. http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@153 PS11, Line 153: T > missing space before Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@286 PS11, Line 286: dequeuing thread > from this context, not clear what thread this is talking about. How about: Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@442 PS11, Line 442: rocessing a query or dml execution request and submitting it to the : /// admission controller > this is confusing since it doesn't actually do that work (it happens asynch Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.h@508 PS11, Line 508: : /// Submits the QuerySchedule to the admission controller and on successful admission, : /// starts up the coordinator execution, makes it accessible by setting : /// 'coord_exec_started_' to true and advances operation_state_ to RUNNING. Handles : /// async cancellation of queries and cleans up state if needed. : void FinishExecQueryOrDmlRequest(); > let's move that to be right after ExecAsyncQueryOrDmlRequest() since it's s Done 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@153 PS9, Line 153: GetCoord > I think you ended up calling it GetCoordinator(), either update the comment Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@173 PS9, Line 173: /// Only set for 'QUERY' and 'DML' statement types and is available only after Exec() > little hard to understand what "set" and "available" mean from just the int Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@176 PS9, Line 176: coord_exec_started_ > I think I was a bit confused about the "started" in the name until I read t Yea, I kinda struggled with naming this as well. http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@182 PS9, Line 182: uses_admission_control > how about: has_async_exec_thread()? does it need to be public? This lingered as a result of how the patch progressed after each iteration. In its current state, it makes sense to just get rid of it and just check if it is null wherever required. (Removed its public use as well and renaming exec_thread_ to async_exec_thread_) http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@324 PS9, Line 324: :unique_ptr<Thread> exec updating this, since it is not quite true, see the updated comment for async_exec_thread. http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@325 PS9, Line 325: : /// Not set for ddl queries. It should only be acce > I think phrasing this in terms of why may help clarify this: Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@327 PS9, Line 327: makes sure that it can onl > FinishExecQueryOrDmlRequest() Done http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/service/client-request-state.h@330 PS9, Line 330: rted_' to true, and : /// UpdateBacken > I think clearer to say: Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@233 PS11, Line 233: UpdateNonErrorOperationState(TOperationState::PENDING_STATE); > The async-exec thread could race ahead and set this to RUNNING before we ge Like you said, the following is the reason I kept it here: "it means that we're guaranteeing that ExecStmt() RPC will always have transitioned away from INITIALIZING before returning" http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@751 PS11, Line 751: operation_state_ < new_state > at this point, is it clearer to just list the possible states? it's just IN Yes, its just those 3 states. Done. http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@915 PS11, Line 915: uses_admission_control() > once renamed, this won't make as much sense. I think we should just uncondi Yes, its harmless to do it unconditionally. Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@1151 PS11, Line 1151: void ClientRequestState::FinishExecQueryOrDmlRequest() { > like the decl, would be nice to move this up to be adjacent to AsyncExecQue Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@1176 PS11, Line 1176: that coord_ is : // visible to the cancellation thread if the query is marked cancelled after exiting : // this critical section. > How about being explicit about why that's important rather than stating jus Done http://gerrit.cloudera.org:8080/#/c/10060/11/be/src/service/client-request-state.cc@1199 PS11, Line 1199: updates are : // only received after Exec() has been called successfully on it > that's not quite accurate. It's more that we these are safe to call concurr Done 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 : /// can be cancelled: either the query is queued or has started executing. > That's kind of secondary. The primary purpose is to have a list of queries on second thought, even "submitted_queries" is not a good name for this. "inflight_queries" works just fine. I still think its worth mentioning what they represent + are used for since "inflight" sounds a bit generic for that purpose. What so you think? http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/10060/9/be/src/util/debug-util.h@117 PS9, Line 117: sleep_option > this is really the code location label or identifier, rather than the optio done, went with sleep_label http://gerrit.cloudera.org:8080/#/c/10060/11/www/query_backends.tmpl File www/query_backends.tmpl: http://gerrit.cloudera.org:8080/#/c/10060/11/www/query_backends.tmpl@90 PS11, Line 90: has completed, or has no backends or has not started : any backends, yet. > the comma locations are kinda weird in this user facing message. rather tha Done -- 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: 11 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:01:34 +0000 Gerrit-HasComments: Yes
