Adar Dembo has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu cluster'

Patch Set 3:

File src/kudu/tools/

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 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
File src/kudu/tools/

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 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to