Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/16220 )
Change subject: WIP CDPD-8989 Improve admission control pool stats logging to be more explicit ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/16220/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16220/1//COMMIT_MSG@7 PS1, Line 7: WIP CDPD-8989 Can you file an upstream JIRA and add that here instead http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/runtime/mem-tracker.h@362 PS1, Line 362: UpdatePoolStatsForQueries I think only this needs to be public, rest can be private if they are only helper functions http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/runtime/mem-tracker.cc@568 PS1, Line 568: : // Recursively append info about this memory tracker and all its children to ss. : void MemTracker::GetAllMemTracker(std::stringstream& ss, int indent) { : ss << std::string(indent, ' ') << " MemTracker: label=" << label_; : : if (!pool_name_.empty()) { : ss << ", pool_name =" << pool_name_; : } : : if (is_query_mem_tracker_) { : ss << ", qid=" << PrintId(query_id_); : } : : ss << std::endl; : indent += 3; : for (MemTracker* child : child_trackers_) { : child->GetAllMemTracker(ss, indent); : } : } : : // Return a debug string for all memory trackers reachable from the root memory : // tracker reachable from this. : string MemTracker::GetAllMemTrackers() { : lock_guard<SpinLock> l(child_trackers_lock_); : std::stringstream ss; : GetRootMemTracker()->GetAllMemTracker(ss, 0); : return ss.str(); : } leftover code from debugging perhaps? http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/scheduling/admission-controller.cc@556 PS1, Line 556: DebugPoolStatsForConsumedMemory Here the check is at the host level, but the log line is produced using pool level stats which is misleading. You will either have to merge the results from all pools like we do for updating HostStats (which might get unwieldy when aggregating the top 5 queries among pools) or add another statestore update that updates host level stats http://gerrit.cloudera.org:8080/#/c/16220/1/be/src/scheduling/admission-controller.cc@1328 PS1, Line 1328: if (tracker) { : // update local_stats_ with the query Ids of the top N queries, plus the min, the max, : // the total memory consumption, and the number of all queries in this pool. : tracker->UpdatePoolStatsForQueries(5 /*limit*/, this->local_stats_); : : } This is being called for the pool level memtracker, which will only give up the top 5 queries in the pool. http://gerrit.cloudera.org:8080/#/c/16220/1/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/16220/1/common/thrift/StatestoreService.thrift@52 PS1, Line 52: 5: required i64 min_memory_consumed; : : // Max memory consumption among all queries. : 6: required i64 max_memory_consumed; Now that i think of this, I am not sure how helpful the min/max memory consumed will be. I think it will be more helpful to add the current consumption of the top K queries and print them instead. http://gerrit.cloudera.org:8080/#/c/16220/1/common/thrift/StatestoreService.thrift@58 PS1, Line 58: 7: required i64 total_memory_consumed; fyi: this can be fetched using the consumption of the parent tracker instead of adding all the queries. Also, seems like this is already tracked locally using metrics_.local_backend_mem_usage at the pool level http://gerrit.cloudera.org:8080/#/c/16220/1/common/thrift/StatestoreService.thrift@60 PS1, Line 60: The current number of requests admitted by any admission controllers : // that are currently running. This is an instantaneous value (as opposed to a : // cumulative sum). I think a more appropriate description will be that this is the num of queries that have live fragments taking up memory on the host. -- To view, visit http://gerrit.cloudera.org:8080/16220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id995a9d044082c3b8f044e1ec25bb4c64347f781 Gerrit-Change-Number: 16220 Gerrit-PatchSet: 1 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 21 Jul 2020 00:36:38 +0000 Gerrit-HasComments: Yes
