Bikramjeet Vig 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 5: (14 comments) 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: /// This tests various valid and invalid cases that the parsing logic in > Add a description comment Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@638 PS3, Line 638: } > Add a description of the test Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@644 PS3, Line 644: gflags::FlagSaver saver; > This is my problem but when I see 'expected' in a test I think it means a v Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@664 PS3, Line 664: } > This sets the flag for all subsequent tests? forgot to remove that, removed http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr-test.cc@682 PS3, Line 682: ExecutorGroup exec_group("foo-group1", 1); > Nit: with more executors? Done 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: parsed_group_prefixes.insert(group_prefix); > Since FE will use ExecutorMembershipSnapshot.exec_group_sets to generate se If we are planning to base the threshold on the resource pool configs then that information can change dynamically whereas an update to FE only happens when there is a change in the cluster state. Considering that, it would be better to fetch the latest resource config while planning and letting the planner arrange the list as it sees fit. 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 addresses in the update to the frontend. For non-default > Nit: addresses in Done 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 will read data remotely and will only send the > Nit: will read Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@588 PS3, Line 588: /// specified we apply the aforementioned steps for each group set. > Nit: steps for Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/scheduling/cluster-membership-mgr.cc@646 PS3, Line 646: LOG(WARNING) << "Some executor groups either do not match expected group sets or " > I wonder if this should be a WARNING level log? Done 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: "'expected_executor_group_sets' which is a more expressive way of specifying " > Maybe explain here how this overrides the value from num_expected_executors Done http://gerrit.cloudera.org:8080/#/c/18093/3/be/src/service/impala-server.cc@297 PS3, Line 297: "For eg. “prefix1:10”, this set will include executor groups named like " > Is there any warning printed in this case? yes, as per your previous suggestion, this will now be logged at warning level instead of info level 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 > I think the only missing piece in TExecutorGroupSet is the threshold. If FE The resource config file is on the local file system which is loaded, parsed and cached into memory. Only changes to the local file trigger a change to the cached config. So any number of fetches for the configs should be quite fast. 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) ? 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: 5 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: Fri, 17 Dec 2021 20:51:57 +0000 Gerrit-HasComments: Yes
