Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 17: Code-Review+2 (3 comments) 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. > This is only accessed by UpdateMembership(),right? I.e. "protected" by upda I guess it kinda is because of the way it is used, but I would still try to avoid mentioning that it "is protected" by the lock, because that might imply that it can 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 anyway we can decompose 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 rid of it altogether or just make 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: 17 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:07:55 +0000 Gerrit-HasComments: Yes
