Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 11: Code-Review+1 (11 comments) Thanks for the reviews. Please see my inline comments and PS12. It also fixes some fallout from the thrift changes in some tests. Carrying Tim's +1. 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@73 PS11, Line 73: /// ClusterMembershipMgr). > indentation is off Done http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@78 PS11, Line 78: Not > Note Done 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 Done 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 y I ended up not liking that mixing at all and actually changed the code to enforce that invariant now. I also clarified it in a comment above the list definitions. http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232 PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to > how to Done http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@232 PS11, Line 232: /// It also serves as an example for how craft statestore messages and pass them to > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@304 PS11, Line 304: mgr > nit: backend Done 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 Done. I added a loop to the end of this test that quiesces the remaining running backends to make sure that executor groups can scale back to 0 element. 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 Done 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 Done. Note that the warnings in L333 and L338 print the state for the backend in the executor group, but I agree that it could have been more clear and added more to the output. 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? I'm not sure I understand the question. In this particular case it won't because we remove a backend that we added in the previous line (MakeBackendDescriptor() creates identical backends when called with the same index parameter). -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 23 May 2019 20:40:06 +0000 Gerrit-HasComments: Yes
