Sahil Takiar 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: (1 comment) 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@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 My understanding of this part of the code is a bit murky, so correct me if I am wrong. I thought that the Coordinator's 'backend_states_' tracks all executing Backends, include the Coordinator Backend (e.g. you can detect if a Backend is running the Coordinator fragment using 'backend->exec_params()->is_coord_backend') (this is the same logic used below to update 'released_coordinator_'), right? ALL_ROWS_PRODUCED wouldn't be a terminal state since it could transition to ERROR, CANCELLED, or RETURNED_RESULTS, so would it make sense to call SetNonErrorTerminalState()? HandleExecStateTransition() also seems to be specific to terminal states - it calls ReleaseExecResources() (maybe calling this early is okay once all results are spooled). The other reason I wanted to add these checks here is to centralize all the logic that decides when admission control resources are released for each backend. -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 21 Aug 2019 19:19:23 +0000 Gerrit-HasComments: Yes
