Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 18: Code-Review+2 (4 comments) Addressed the last comments, carrying Bikram's +2. http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.h@210 PS17, Line 210: /// post-recovery grace period. Not exposed to clients and not protected by any locking. > I guess it kinda is because of the way it is used, but I would still try to My intention with that comment was the same as Bikram's, the protection is just a coincidence so to speak, but it's not expected to be modified concurrently. http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@86 PS17, Line 86: UpdateMembership > nit: not a huge deal but I feel this method has become too long, is there a I agree, it's getting close to 200 lines. It has plenty of comments though and the code is rather linear. We could try to factor out some of the blocks in the core loop and updating the local backend if you think that helps. A lot of the top-level variables and flags are used throughout the method though. >From your +2 I assume you're OK with doing it in a subsequent change? That >would allow us to move forward with a handful of dependent changes (which >likely won't have to touch this method) and will make the resulting changes >easier to review, too. http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/scheduling/cluster-membership-mgr.cc@272 PS17, Line 272: if (update_local_server) NotifyLocalServerForDeletedBackend(*new_backend_map); > I guess this means we call NotifyLocalServerForDeletedBackend() in more cas Yes, historically we called it for every update. As an attempt to make it more efficient we try not to for some cases but we don't have to be exact about it. http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/13207/17/be/src/statestore/statestore-subscriber.cc@467 PS17, Line 467: MilliSecondsSinceLastRegistration > nit: I think we dont need to expose this method anymore. we can just get ri Made it private. -- 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: 18 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: Fri, 31 May 2019 00:20:52 +0000 Gerrit-HasComments: Yes
