Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13868 )
Change subject: IMPALA-8339: Add local executor blacklist to coordinators ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/13868/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13868/6//COMMIT_MSG@23 PS6, Line 23: a straggler. > Mention the change to the profile in the commit message? Done http://gerrit.cloudera.org:8080/#/c/13868/6//COMMIT_MSG@28 PS6, Line 28: > Are you planning to do these in this change? I was intending do this as follow up work http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@350 PS6, Line 350: const int BLACKLIST_TIMEOUT_SLEEP_US = 100000; > const int, uppercase? I think that the frequencies could also be closer to Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@354 PS6, Line 354: EXPECT_EQ(NUM_BACKENDS, backends_.size()); > nit: single line Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@374 PS6, Line 374: NU > nit: BE, or backend. I tripped over "a be to" :) Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@415 PS6, Line 415: > nit: extra word? Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.h File be/src/scheduling/executor-blacklist.h: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.h@39 PS6, Line 39: probation > I think "greylisting" is another term that we could use which sounds less l I think 'probation' is clearer since its a more common term, but willing to change it if others want http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.h@43 PS6, Line 43: When > When Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.cc File be/src/scheduling/executor-blacklist.cc: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.cc@86 PS6, Line 86: } > Does this not need to check the timeouts? Or do we assume that the caller c ClusterMembershipMgr uses this to determine if the removed BE would be present in the snapshot's 'executor_groups', i.e. if it was previously blacklisted it won't be in 'executor_groups'. We only transition things to probation during Maitenance() because we need to do the 'unblacklisting' step at the same time to keep things consistent. http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/executor-blacklist.cc@191 PS6, Line 191: g re > This can be a variable for clarity. Done http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/statestore/statestore.h@183 PS6, Line 183: time > Please specify the time unit. Done -- 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: 7 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 29 Jul 2019 18:13:24 +0000 Gerrit-HasComments: Yes
