Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14103 )
Change subject: IMPALA-8858: Add observability of the query load on executor groups ...................................................................... Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@364 PS8, Line 364: : TUpdateExecutorMembershipRequest update_req; : for (const auto& it : snapshot->current_backends) { : const TBackendDescriptor& backend = it.second; : if (backend.is_executor) { : update_req.hostnames.insert(backend.address.hostname); : update_req.ip_addresses.insert(backend.ip_address); : update_req.num_executors++; : } : } : Status status = this->frontend()->UpdateExecutorMembership(update_req); : if (!status.ok()) { : LOG(WARNING) << "Error updating frontend membership snapshot: " : << status.GetDetail(); : } I'm inclined to think that this code should not be in the ExecEnv (but we can postpone that to a cleanup change). I'm aware that the typedefs inside the CMM cannot be forward-declared and we shouldn't include it in the frontend, but we should look into pulling the typedefs out of the CMM and then simplify this code by moving it into the destination of the callbacks. http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@494 PS8, Line 494: std:: nit: remove std:: in .cc files http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/runtime/exec-env.cc@496 PS8, Line 496: current_backend_set.insert(it.second.address); Same as comment in the other callback http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/admission-controller.h@948 PS8, Line 948: load metric of the 'grp_name' executor group by 'delta' While reading this I was confused what 'delta' is, maybe instead of "query load" we should use "query count" (load is often a value between 0 and 1)? 'num_queries' might also be a good parameter name if we rename the method to something like "IncExecGroupMetricLocked()". http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.h@124 PS8, Line 124: typedef std::function<void(SnapshotPtr)> UpdateCallbackFn; Previously, UpdateLocalServerFn was only called when backends had been deleted. I think it would be nice to preserve that, e.g. by adding a bool can_contain_deletes (feel free to find a better name) to the callback. http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@321 PS8, Line 321: SetStateAndUpdateMetrics(new_state); I know I asked for the metric update to be moved there, but now that we generalized the callback mechanism, I think it could be nice to update the metrics in a callback. What do you think? http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@322 PS8, Line 322: nit: extra space http://gerrit.cloudera.org:8080/#/c/14103/8/be/src/scheduling/cluster-membership-mgr.cc@323 PS8, Line 323: NotifyAllCallbackUpdateFns I think NotifyListeners(new_state) might be more concise but clear enough http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/14103/6/common/thrift/metrics.json@2502 PS6, Line 2502: ad.$0" > do you think num-running-queries is more appropriate here? maybe even num-running is ok (see my comment elsewhere about "load"). How do we call other metrics that track number of queries? -- To view, visit http://gerrit.cloudera.org:8080/14103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58cde8699c33af8b87273437e9d8bf6371a34539 Gerrit-Change-Number: 14103 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Comment-Date: Thu, 05 Sep 2019 21:59:43 +0000 Gerrit-HasComments: Yes
