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 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc@1147 PS7, Line 1147: lock_guard<mutex> l(lock_); : if (!UpdateQueryStatus(admit_status).ok()) return; : if (is_cancelled_) { : VLOG_QUERY << "Cancelled right after successful admission, query id=" : << schedule_->query_id(); : // If query cancelled after being admitted, release Admission control resources. : exec_env_->admission_controller()->ReleaseQuery(*schedule_.get()); : // If cancellation was user initiated, then query_status would be OK, therefore : // set it to CANCELLED. : discard_result(UpdateQueryStatus(Status::CANCELLED)); : return; : } > Yes this is for the small window where cancellation is initiated after the Given how small the window is, I think it's better to go with the simpler approach. It makes less cases that need to be covered in tests. Removing this special case is equivalent to the Cancel()ing thread happening just slightly later, right? -- 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: 7 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: Fri, 11 May 2018 23:49:53 +0000 Gerrit-HasComments: Yes
