Todd Lipcon has posted comments on this change.

Change subject: ksck: colorize and clean up output

Patch Set 2:

File src/kudu/tools/

PS2, Line 28: ksck
> This doesn't seem to just apply to ksck though; the way you've defined it, 

PS2, Line 60: COLOR_RED
> If you use an enum class instead of an enum, you'll get better scoping.

Line 64:     case COLOR_RESET: return "\x1b[m";
> Might be useful to add a comment here with a source for the color codes.
File src/kudu/tools/

Line 586:   return good_table;
> Nit: you could just return true/false out of the if/else branches and drop 

PS2, Line 602: const shared_ptr<KsckTabletReplica>
> Nit: const auto&
File src/kudu/tools/ksck.h:

PS2, Line 389:       
> indent
File src/kudu/tools/

Line 140:     .AddOptionalParameter("color")
> Nit: let's keep these sorted alphabetically.
Done, though I don't know if sorting is always the best. eg git seems to group 
its usage output by "function":

usage: git [--version] [--help] [-C <path>] [-c name=value]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           <command> [<args>]

I think that makes some sense, but not gonna re-organize for now.

To view, visit
To unsubscribe, visit

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

Reply via email to