Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10536 )
Change subject: [tools] add 'kudu tablet set_attributes' sub-command ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/10536/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10536/1//COMMIT_MSG@7 PS1, Line 7: kudu CLI > I usually use [tools] for this. Done http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc@599 PS1, Line 599: *change->mutable_peer()->mutable_permanent_uuid() > Can use set_permanent_uuid(). Done http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc@600 PS1, Line 600: change->mutable_peer()->mutable_attrs()->set_promote(FLAGS_attribute_promote); : change->mutable_peer()->mutable_attrs()->set_replace(FLAGS_attribute_replace); > Does it make sense to set both? What would happen? Should the tool reject t It's a good catch -- yes, setting both PROMOTE and REPLACE does not make sense. I updated the code to return InvalidArgument status in that case. >From what I saw in raft_conensus.cc, right now the server side does not impose >any restrictions or sanity checks on the attribute combinations. http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc@606 PS1, Line 606: RETURN_NOT_OK(proxy->BulkChangeConfig(req, &resp, &rpc)); : if (resp.has_error()) { : // The status of the request might be InvalidArgument() if the : // replica already has the attributes set as specified in the incoming : // request. : return StatusFromPB(resp.error().status()); : } : : return Status::OK(); > aside: I feel like I see this pattern a lot. I wonder if it'd be worth a ma You are right -- this pattern happens a lot. I think it makes sense to have a macro for that, yes. Probably, in a separate changelist? http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc@675 PS1, Line 675: /reset > We can only set. Done http://gerrit.cloudera.org:8080/#/c/10536/1/src/kudu/tools/tool_action_tablet.cc@676 PS1, Line 676: replica > The BulkConfigChange is replicated, right? So it's the tablet's configurati Right, thanks -- that's a better wording. -- To view, visit http://gerrit.cloudera.org:8080/10536 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib304715100ba9062558863f140aa309fd604ace3 Gerrit-Change-Number: 10536 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 06 Jun 2018 17:28:14 +0000 Gerrit-HasComments: Yes
