Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188 PS8, Line 188: RemoveExecutor Annoyingly, this call to RemoveExecutor() (and the others below) are wrong. The executor group code uses equality checks to find executors to remove them, and if is_quiescing changes, that won't work. To solve this I can think of several options: * 1) Check be_desc.address.port and be_desc.ip_address inside RemoveExecutor (krpc_address could also be a key). This makes the code most flexible. * 2) DCHECK when calls to RemoveExecutor() don't result in an element being removed (and warn in release builds) * 3) Switch the call sites here to pass in the existing be_desc instead of the new one. Both 1) or 3) would fix this, and 2) could provide more guard rails to prevent this in the future. I'm leaning towards 1) because it makes the ExecutorGroup class easier to use and we never should have two be_descs for the same IP:port combination. What do others think? http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299 PS8, Line 299: return true; This method lacks a check to compare the local be desc inside current_backends (if it exists) to local_be_desc. -- 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: 8 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, 09 May 2019 17:31:11 +0000 Gerrit-HasComments: Yes
