Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 )
Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/14104/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14104/1//COMMIT_MSG@24 PS1, Line 24: * Resources are released at most every 1 seconds, this prevents short at most once http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@403 PS1, Line 403: UNRELEASED Will "IN_USE" be slightly clear ? http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@481 PS1, Line 481: RELEASE_BACKEND_STATES_DELAY Please add _MS suffix http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@505 PS1, Line 505: num_not_unreleased_ nits: double-negative may not be very comprehensible. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@823 PS1, Line 823: num_pending_++ Coding convention for Impala recommends pre-increment http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@846 PS1, Line 846: last_backend || coordinator_last_backend It seems a bit ad-hoc to detect these cases here in this function. I wonder if it makes sense to have a new state in coordinator called "ALL_ROWS_PRODUCED / EOS" which is accessible if result spooling is enabled. Once reaching this state, the thread will call SetNonErrorTerminalState() which end up calling HandleExecStateTransition(). In this way, we can handle these cases in ReleaseAdmissionControlResources(), which seems more natural as it marks the end of all non-coordinator fragments. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc@370 PS1, Line 370: DCHECK(false) << strings::Substitute( : "Error: Cannot find host address $0 for query $1.", PrintThrift(host_addr), : PrintId(schedule.query_id())); What happens in non-debug builds ? Should we log here instead and add a continue statement ? We can keep the DCHECK(false) though. -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Aug 2019 18:47:31 +0000 Gerrit-HasComments: Yes
