Adar Dembo has posted comments on this change. Change subject: tool: port kudu-admin to 'kudu cluster' ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/4180/3/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: PS3, Line 57: kAdminToolName > As I can see we are omitting the word 'admin' altogether, so we can remove Eh, I didn't feel strongly about this. Yes, it's a little confusing, and perhaps the test should be renamed. But "kudu-cluster-test" doesn't necessarily imply a connection to tool_action_cluster.cc either, plus I expect that some actions may move under other modes in the future. Do we rename the tests when that happens? In short, I followed the principle of "change the least amount of code" when updating kudu-admin-test, hence why I kept all the variable names more or less the same. And I don't think it's necessarily wrong that the variable names are things like kAdminToolName; there is still an "admin" tool, it's just part of "kudu" now. PS3, Line 184: TestTable > We seem to have a const under the tserver namespace from tablet_server-test Right, I should have seen that above. Fixed. PS3, Line 204: nullptr > I wonder if ASSERT_OK grab the stderr output from Subprocess:Call ? Here we There's going to be a lot of stderr output though, from the admin tool starting up and sending RPCs. I can't test that it's empty, and I don't think it's really interesting to test the log messages that come out. We're already doing ASSERT_OK() on the Call() itself. PS3, Line 209: TestTable > same as above Done http://gerrit.cloudera.org:8080/#/c/4180/3/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: Line 293: unique_ptr<Action> delete_table = > nit: we could alphabetically order these actions, so when help string is em But they are already alphabetically sorted, aren't they? Or do you mean I should move change_config up to before delete_table? OK, I can do that. Line 374: unique_ptr<Mode> change_config = > nit: we can perhaps keep this submode creation under different function cal Personally I don't find this many lines to be that bad for one function, since many are boilerplate. We can revisit that down the line, of course. Line 384: .AddAction(std::move(delete_table)) > I'm a little unconvinced about mixing "physical"/"operator" type operations I'm not sure. I'd prefer to keep the grouping that we have now so that it's at least internally consistent (even if you're right and it's ultimately confusing to different kinds of users). To reiterate (for others' benefit), the grouping that I've used is based on scope of work (i.e. operating on a single tablet, operating on the entire cluster, etc.) and similarity of command line arguments. The latter especially, since it lends itself well to implementing argument sharing at the mode level. So to summarize, I'd prefer to keep the grouping as-is right now, and once we're done with all of the porting, take a step back and reevaluate, perhaps with the guidance of Kudu users (as opposed to us developers) to drive the grouping. -- To view, visit http://gerrit.cloudera.org:8080/4180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
