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 4: (24 comments) Addressed all pending comments (I think). Lots of changes so re-running core tests. The test failures I was seeing earlier are resolved now. Added some more DCHECKs in Coordinator / AdmissionController to make sure no resources are being leaked. http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state-test.cc File be/src/runtime/coordinator-backend-state-test.cc: http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state-test.cc@113 PS2, Line 113: backend_state.second, Coordinator::BackendResourceState::ResourceState::IN_USE); > Calling Close() multiple times feels a bit weird to me since it doesn't rea Exposed backend_resource_states_ directly. http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.h@472 PS2, Line 472: /// Closes that state machine and returns a vector of IN_USE or PENDING BackendStates. > Can you document that it's idempotent, and also if the contract is that the Done http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.h@477 PS2, Line 477: private: > I think we should consider making this and BATCHED_RELEASE_DECAY_FACTOR hid Done http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.h@512 PS2, Line 512: > maybe int64_t? I don't think this is likely to overflow but would be nice t Done 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@63 PS1, Line 63: > nit: we can get the query_id and BackendExecParams from the QuerySchedule yeah, we can derive query_id from the QuerySchedule. I'm not sure if it makes sense to derivce the BackendExecParams though? QuerySchedule::per_backend_exec_params() exposes all the BackendExecParams, but they are indexed by a TNetworkAddress, which the constructor does not have access to. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@403 PS1, Line 403: > +1 Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@413 PS1, Line 413: are in the RELEAS > nit: transition to? Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@469 PS1, Line 469: ackendStates ha > nit: MarkBackendFinished Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@470 PS1, Line 470: vector<BackendState*>& released_backend_states); > nit: I usually try to avoid (input,output) type params if possible. Maybe c I think there are some possible race conditions with adding an extra GetReleasableBackends() method because then multiple threads could read the same list of RELEASABLE backends and both decide to release them. The reason I put it all in one method is to avoid this. We could return a vector of releasable_backend_states instead? http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@474 PS1, Line 474: ackendStates as > nit: how about ReleaseBackends? MarkBackendsReleased? It's not actually releasing any resources, its just managing the state. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@477 PS1, Line 477: e: > +1 to returning the vector instead of requiring the caller pass it in, I th Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@492 PS1, Line 492: > nit: space Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@527 PS1, Line 527: friend class CoordinatorBackendStateTest; > It's owned by the Coordinator, updated the comments with the ownership info Done 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@827 PS1, Line 827: > nit: how about is_last_unrelease_backend I ended up removing this check entirely because it is not necessary and just adds an additional call to admission control (if this is the last backend to complete, then the coordinator will call ReleaseQueryAdmissionControlResources which releases any remaining resources). http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@827 PS1, Line 827: > nit: use num_backends_ here and below Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@831 PS1, Line 831: ard<SpinLock> lock(lock_ > nit: how about is_coord_the_last_unreleased_backend Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@836 PS1, Line 836: m_pending_; > should be released_timer_.ElapsedTime() > RELEASE_BACKEND_STATES_DELAY * 10 Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@846 PS1, Line 846: is_coordinator_the_last_unreleased_back > Agree that it might be clearer to add a new state to the state machine to m Yeah I think that all makes sense. Doing it as part of IMPALA-6984 makes sense as well. I think these changes will have the most impact there. Assigned IMPALA-6984 to myself. http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/2/be/src/runtime/coordinator-backend-state.cc@824 PS2, Line 824: // destructor runs. > What would happen if we enabled this when query result spooling was disable Yeah, I sorta had been going back and forth on this, but I was thinking about this earlier and I agree, I think it will be beneficial when result spooling is disabled as well. IIRC the kRPC receiver queues several RowBatches, so its likely that it already buffers almost all results for a query; and any query that returns under BATCH_SIZE rows would benefit from this as well. http://gerrit.cloudera.org:8080/#/c/14104/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/3/be/src/runtime/coordinator-backend-state.cc@897 PS3, Line 897: vector<Coordinator::BackendState*> > line too long (103 > 90) Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator.cc@764 PS1, Line 764: : // Mark backend_state as closed and release the backend_state's resources if : // necessary. : vector<BackendState*> releasable_backends; : backend_resource_state_->MarkBackendFinished(backend_state, &releasable_backends); : i > is it possible that both this thread and a cancellation thread call Release It shouldn't because Close() only returns all IN_USE or PENDING backends. This code code can only return / release RELEASABLE backends. This is essentially the reason the RELEASABLE state is necessary in the first place. 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@367 PS1, Line 367: // Update stats tracking memory admitted. : DCHECK_GT(mem_to_release, 0); : local_mem_admitted_ -= mem_to_release; > nit: we can probably calculate this in UpdateHostStatsForQueryBackends and Done, renamed this method to ReleaseMem(int64_t mem_to_release) since that is all it does now. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc@410 PS1, Line 410: = GetMemToAdmit(schedule, back > maybe worth mentioning in its comment that this only used to release the qu Done http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc@420 PS1, Line 420: const TNetworkAddress& host_addr = entry.first; : int64_t mem_to_admit = GetMemToAdmit(schedule, entry.second); : if (!is_admitting) mem_to_admit *= -1; : int num_queries = is_admitting ? 1 : -1; : UpdateStatsForHost(host_addr, mem_to_admit, num_queries); : } : } : : void AdmissionController::UpdateStatsForHost( : const TNetworkAddress& host_addr, int64_t mem_to_admit, int num_queries_to_admit) { : const string host = TNetworkAddressToString(host_addr); : VLOG_ROW << "Update admitted mem reserved for host=" << host : << " prev=" << PrintBytes(host_stats_[host].mem_admitted) : << " new=" << PrintBytes(host_stats_[h > nit: this is the same case as in UpdateHostStats() maybe refactor it out an Done -- 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: 4 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: Tue, 27 Aug 2019 22:44:41 +0000 Gerrit-HasComments: Yes
