Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13868 )
Change subject: [WIP] IMPALA-8339: Add local node blacklist to coordinators ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.h@168 PS1, Line 168: /// Add the given backend to the local blacklist. Update 'current_membership_' to remove nit: Adds, Updates. http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.h@170 PS1, Line 170: void Blacklist(const TBackendDescriptor& be_desc); Maybe rename to BlacklistExecutor? Should this method accept an IP address instead? http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.cc@271 PS1, Line 271: I think the overall control flow could be simpler if we apply all SS updates first, irrespective of blacklisting, and then apply all blacklisting information in a subsequent step here. In other words, we first assemble the full SS view, then we modify it according to the local view. http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/cluster-membership-mgr.cc@330 PS1, Line 330: which doesn't apply here Maybe spell out explicitly why it doesn't apply? http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.h File be/src/scheduling/node-blacklist.h: http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.h@48 PS1, Line 48: class NodeBlacklist { > Is this class called only from the ClusterMembershipMgr class ? If so, may On the other hand the CMM is already pretty large http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.h@57 PS1, Line 57: Remove Maybe FindAndRemove would make the usage more clear when calling this? http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.h@81 PS1, Line 81: bool blacklisted; Could also be an enum? http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.h@92 PS1, Line 92: std::unordered_map<IpAddr, std::vector<Entry>> node_list_; Have you considered blacklisting whole hosts? http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.cc File be/src/scheduling/node-blacklist.cc: http://gerrit.cloudera.org:8080/#/c/13868/1/be/src/scheduling/node-blacklist.cc@112 PS1, Line 112: unblacklisted std::move() or return values through pointer -- To view, visit http://gerrit.cloudera.org:8080/13868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacb6e73b84042c33cd475b82470a975d04ee9b74 Gerrit-Change-Number: 13868 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Fri, 19 Jul 2019 20:12:54 +0000 Gerrit-HasComments: Yes
