Tim Armstrong 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 (7 comments) Thank you for the changes. This is a vast improvement to this area of the code. I think Bikram should re-review the changes since his +1 because a lot changed here. http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc File be/src/scheduling/cluster-membership-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/13207/10/be/src/scheduling/cluster-membership-mgr-test.cc@384 PS10, Line 384: /// removed from the cluster after having been quiesced before, or removed from the > That's what I meant by "killing" a backend below (L391). I added a comment Ah great, I missed that. 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 http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@78 PS11, Line 78: Not Note http://gerrit.cloudera.org:8080/#/c/13207/11/be/src/scheduling/cluster-membership-mgr-test.cc@80 PS11, Line 80: destructed I was going to say that this should be "destroyed", but I googled and there's a lot of people on the internet arguing about this, so ¯\_(?)_/¯ 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 how to http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67 PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr; > Any preference here? BeDescSharedPtr sounds more natural to me. http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79 PS8, Line 79: // implicitly-defined default and copy constructors. > That makes sense, thanks for the explanation. What do you think about the d I think that's good. -- 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: Sat, 18 May 2019 18:29:29 +0000 Gerrit-HasComments: Yes
