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 6: (4 comments) Thanks a lot for the update! http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@573 PS5, Line 573: first > just to clarify, the ExpectedExecGroupSets is actually a typedef of vector< Can we use TExecutorGroupSet which covers all the members needed? See my other comment on the same topic. http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr-test.cc@659 PS5, Line 659: EXPECT_EQ(update_req.exec_group_sets.size(), 1); : EXPECT_EQ(update_req.exec_group_sets[0].curr_num_executors, 1); : EXPECT_EQ(update_req.exec_group_sets[0].expected_num_executors, 20); : EXPECT_EQ(update_req.exec_group_sets[0].exec_group_name_prefix, ""); > just to clarify, do you want me to add separate JUnit tests that mimic thes not really. I think adding some extra assertions in this test is sufficient. http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/18093/5/be/src/scheduling/cluster-membership-mgr.h@222 PS5, Line 222: expected_exec_group_sets In this patch we have two objects defined: TExecutorGroupSets and ExpectedExecGroupSets and ExpectedExecGroupSets is a subset of TExecutorGroupSets structure wise: the name, the current size and expected size vs. the name and expected size. I recall I had the question about whether we ever need ExpectedExecGroupSets during early review. It seems to me there are certain benefits to uniform these two objects into TExecutorGroupSets only. 1. Simplicity and consistency; 2. Easy specification of a complete group sets (i.e. regular:3:3, large:20:10, where the newly added :<n> is for curr_num_executors) during cluster startup time (particularly useful to integration tests and simulation of customer issues on a dev box). I'd like to bring this up and hear your opinion. http://gerrit.cloudera.org:8080/#/c/18093/5/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/5/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@97 PS5, Line 97: String host = address.getHostname(); > exec_group_sets will always be non empty for any kind of update. If using ' Done -- 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: 6 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 22 Dec 2021 14:57:43 +0000 Gerrit-HasComments: Yes
