Todd Lipcon has posted comments on this change.

Change subject: KUDU-1474: single to multi-master deployment migration

Patch Set 6:

File src/kudu/gutil/strings/join.h:

Line 204: string JoinStrings(const CONTAINER& components,
some comment doc here would be good. Perhaps a different name as well, like 
'JoinMapped' or 'MapJoinStrings' or somesuch?
File src/kudu/tools/

Line 45:   msg += "Action can be one of the following:\n";
this function only seems useful/sensical if 'sub_actions' is non-empty, but on 
line 63 it gets called with empty sub_actions
File src/kudu/tools/

Line 124:   RETURN_NOT_OK(env_util::CopyFile(env, cmeta_filename, 
backup_filename, opts));
worth a LOG(INFO) about 'backing up current config to <filename>'
File src/kudu/tools/

Line 55:     return 0;
> While I agree in principle, won't such a test have to be updated with every
yea, though the test could use 'STR_CONTAINS' assertions just to look for 
whatever first subtool we have. just noticed in the past that these sorts of 
tools are used infrequently enough that they tend to have breakages we only 
notice much later.

The fact that there's coverage for the "happy path" is nice, but some basic 
error-path coverage even if the assertions are loose would be good.
File src/kudu/tools/

Line 69:     vector<Action> sub_actions = cur->sub_actions;
is this mutated? or can it be const auto&?

Line 130:       FLAGS_helpfull ||
it seems like it would be useful to still allow --helpfull or something to get 
at the real help

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to