Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8619 )
Change subject: WIP [catalog_manager] added 3-4-3 behavior ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/consensus/quorum_util.cc@421 PS2, Line 421: peer.attrs().has_promote() > I don't think we need to rely on has_ since this defaults to false. Done http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/consensus/quorum_util.cc@450 PS2, Line 450: for (const RaftPeerPB& peer : config.peers()) { > I find the logic a little tough to follow in here. Maybe it would be a litt I thought about that but it would require keeping the finding in the temporary containers, etc. while it's possible to do so without allocating an extra stuff. But if it would be easier to follow -- sure, I'll update that as needed. Yeah, that a good point about using the same code for deciding to evict either a voter or non-voter. The logic in the functions to evict voter and non-voters was using different criteria in the first version, but now it's the same. I'll update that, thanks. http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/master/catalog_manager.cc@204 PS2, Line 204: // TODO(aserbin): can we get a better name for this flag? Ideally, there should > i posted a patch at https://gerrit.cloudera.org/c/8626/ so maybe we can use thanks! that would be a good thing to have. http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/master/catalog_manager.cc@3007 PS2, Line 3007: member_type_(member_type) > this field only seems relevant for adding new replicas or a forced promotio good catch -- i'll move it into appropriate function. http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/master/catalog_manager.cc@3198 PS2, Line 3198: peer->set_member_type(member_type_); > why is this field needed for an eviction? Good point: I think it's not needed for eviction. I'll move it into the AsyncAddReplicaTask. http://gerrit.cloudera.org:8080/#/c/8619/2/src/kudu/master/catalog_manager.cc@3482 PS2, Line 3482: else { : string to_evict; : if > nit: we can make this an "else if" and avoid an additional level of nesting I was trying to keep to_evict local. All right, having less level of nesting is a priority makes more sense, I'll switch to 'else if'. -- To view, visit http://gerrit.cloudera.org:8080/8619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6f0469ac641bf7a03dbef01eaa3f1b58a5bf5d27 Gerrit-Change-Number: 8619 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 22 Nov 2017 01:40:52 +0000 Gerrit-HasComments: Yes
