Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@120 PS11, Line 120: int num_expected_updates = -1 nit: unused http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@149 PS11, Line 149: but does not remove the : /// backend from that list after creating its CMM. so those lists are not exclusive, that is, their members can overlap. Can you mention that briefly because after reading "Backends can be in one of 5 states" my initial thought was that those lists maintain this invariant http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@304 PS11, Line 304: mgr nit: backend http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@342 PS11, Line 342: // Quiesce them all how about Quiesce half of it, to exercise the path where backends later get deleted directly. Although i know that would be covered by the next test, it might still make sense to go through it deterministically http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@391 PS11, Line 391: P_SHUTDOWN nit: maybe call it P_QUIESCE to be consistent http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr.cc@350 PS11, Line 350: : is_quiescing" nit: maybe print here and below what the value is for group_be.is_quiescing http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc File be/src/scheduling/executor-group-test.cc: http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/executor-group-test.cc@61 PS11, Line 61: executor_group.RemoveExecutor(MakeBackendDescriptor(2)); will the LOG DFATAL result in an exit here and cause the test to fail? -- To view, visit http://gerrit.cloudera.org:8080/13207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3 Gerrit-Change-Number: 13207 Gerrit-PatchSet: 11 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@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 Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 21 May 2019 20:56:15 +0000 Gerrit-HasComments: Yes
