Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-admin to 'kudu cluster'
Patch Set 3:
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
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
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
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 http://gerrit.cloudera.org:8080/4180
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>