Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13868 )
Change subject: IMPALA-8339: Add local executor blacklist to coordinators ...................................................................... Patch Set 6: (8 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? http://gerrit.cloudera.org:8080/#/c/13868/6//COMMIT_MSG@28 PS6, Line 28: TODO Are you planning to do these in this change? 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: int blacklist_timeout_sleep_us = 5000; const int, uppercase? I think that the frequencies could also be closer to the real world ones, given we only sleep 3 times during this test. http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@354 PS6, Line 354: CreateBackend(); nit: single line http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@374 PS6, Line 374: be nit: BE, or backend. I tripped over "a be to" :) http://gerrit.cloudera.org:8080/#/c/13868/6/be/src/scheduling/cluster-membership-mgr-test.cc@415 PS6, Line 415: it nit: extra word? 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 like law enforcement, but I don't feel strongly about it. 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: State removed_state = remove_it->state; Does this not need to check the timeouts? Or do we assume that the caller called Maintenance() before calling this method? -- 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: 6 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: Sat, 27 Jul 2019 03:40:27 +0000 Gerrit-HasComments: Yes
