Andrew Sherman 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 3: (13 comments) Looks good, I have a few nits and questions http://gerrit.cloudera.org:8080/#/c/18093/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18093/3//COMMIT_MSG@11 PS3, Line 11: size. Every executor group that is a part of this set will have the Somewhere I think you should say that the 'executor group size' means the number of executors in the group. http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@554 PS3, Line 554: TEST(ClusterMembershipMgrUnitTest, TestPopulateExpectedExecGroupSets) { Add a description comment http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@638 PS3, Line 638: TEST(ClusterMembershipMgrUnitTest, PopulateExecutorMembershipRequest) { Add a description of the test http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@644 PS3, Line 644: ClusterMembershipMgr::ExpectedExecGroupSets empty_expected_exec_group_sets; This is my problem but when I see 'expected' in a test I think it means a value that will be used in an assertion to validate some value http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@664 PS3, Line 664: FLAGS_expected_executor_group_sets = "foo:2,bar:10"; This sets the flag for all subsequent tests? Maybe this should be set outside the block (like on line 661) http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@682 PS3, Line 682: // Adding another exec group with a more executors. Nit: with more executors? http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@585 PS3, Line 585: /// include the hostnames and IP addressesin the update to the frontend. For non-default Nit: addresses in http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@586 PS3, Line 586: /// executor groups, we assume that we willread data remotely and will only send the Nit: will read http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@588 PS3, Line 588: /// specified we apply the aforementioned stepsfor each group set. Nit: steps for http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@646 PS3, Line 646: LOG(INFO) << "Some executor groups either do not match expected group sets or " I wonder if this should be a WARNING level log? http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc@291 PS3, Line 291: DEFINE_string(expected_executor_group_sets, "", Maybe explain here how this overrides the value from num_expected_executors if both are set? We should note that here or (perhaps better) in DEFINE_int32(num_expected_executors http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc@297 PS3, Line 297: "executor groups that do not map to the specified group sets will never be used to " Is there any warning printed in this case? Would it be easy to do so? http://gerrit.cloudera.org:8080/#/c/18093/3/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/3/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@71 PS3, Line 71: private final List<TExecutorGroupSet> exec_group_sets; Should this be exec_group_sets_ (i.e with a trailing underscore) ? -- 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: 3 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: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 16 Dec 2021 02:18:13 +0000 Gerrit-HasComments: Yes
