Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18093 )
Change subject: IMPALA-11033: Add support for specifying multiple executor group sets ...................................................................... Patch Set 2: (5 comments) Looks good! http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@218 PS2, Line 218: x and expected group size It will be nice if the current #executors is stored in this data structure. http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.h@229 PS2, Line 229: expected_ I wonder if we need to use 'expected' in the name. To me, the concept of a list of executor group sets is to be handled. http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18093/2/be/src/scheduling/cluster-membership-mgr.cc@685 PS2, Line 685: sort by increasing order expected group size. How do we deal with a common situation where a small number of exec groups are down? Say out of 10 groups, 2 are down. In this case, I think FE should target the queries to the 8 running groups. When all are down, then FE considers all based on expected executor size. Procedurally, it is 1. When expected_exec_group_sets (prefer to call it exec_group_sets) has at least one healthy group, sort all healthy groups based on current executor size; 2. Otherwise, sort based on expected executor size http://gerrit.cloudera.org:8080/#/c/18093/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/18093/2/common/thrift/Frontend.thrift@720 PS2, Line 720: no healthy executor groups are present nit. "curr_num_executors is 0" is better and clear. http://gerrit.cloudera.org:8080/#/c/18093/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java: http://gerrit.cloudera.org:8080/#/c/18093/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@71 PS2, Line 71: exec_group_sets Need a call like List<TExecutorGroupSet> getExecGroupSets() { return exec_group_sets; } to expose the sets to FE. -- To view, visit http://gerrit.cloudera.org:8080/18093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e0a3a5fe2b1f0b7507b7c096b7a3c373bc2e684 Gerrit-Change-Number: 18093 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Tue, 14 Dec 2021 18:22:35 +0000 Gerrit-HasComments: Yes
