Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22249 )
Change subject: IMPALA-13201: System Table Queries Execute When Admission Queues are Full ...................................................................... Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/admission-controller.cc@2271 PS7, Line 2271: > I think I prefer the old method than the new one. The old method is clear, I modified the code so that all_coordinators is now a plain ExecutorGroup instance that is managed via ClusterMembershipMgr::UpdateMembership just like all the other cluster state. http://gerrit.cloudera.org:8080/#/c/22249/8/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/22249/8/be/src/scheduling/cluster-membership-mgr.h@115 PS8, Line 115: ExecutorGroup all_coordinators; > This is an optimization to avoid having to iterate all backends to construc Yes, since an ExecutorGroup containing all coordinators is used both in admission control and in scheduling, this optimization prevents having to iterate through all nodes each time the list of all coordinators is needed. http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/22249/7/be/src/scheduling/cluster-membership-mgr.h@115 PS7, Line 115: ExecutorGroup all_coordinators > Can this just be ExecutorGroup and not shared_ptr? Done. all_coordinators is managed in ClusterMembershipMgr::UpdateMembership where the other cluster state is already managed. http://gerrit.cloudera.org:8080/#/c/22249/8/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/22249/8/be/src/scheduling/cluster-membership-mgr.cc@202 PS8, Line 202: } > Why remove and re-add, rather than add only if it's absent or make AddExecu I eliminated this function. It's only necessary to do the remove before add when processing a local backend update since that section of the code is called when the local backend is both created and updated. -- To view, visit http://gerrit.cloudera.org:8080/22249 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e0e64db92bdbf80f8b5bd85d001ffe4c8c9ffda Gerrit-Change-Number: 22249 Gerrit-PatchSet: 12 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 05 Feb 2025 19:30:01 +0000 Gerrit-HasComments: Yes
