Todd Lipcon has posted comments on this change. Change subject: ksck: colorize and clean up output ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/4129/2/src/kudu/tools/color.cc File src/kudu/tools/color.cc: PS2, Line 28: ksck > This doesn't seem to just apply to ksck though; the way you've defined it, Done PS2, Line 60: COLOR_RED > If you use an enum class instead of an enum, you'll get better scoping. Done Line 64: case COLOR_RESET: return "\x1b[m"; > Might be useful to add a comment here with a source for the color codes. Done http://gerrit.cloudera.org:8080/#/c/4129/2/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: Line 586: return good_table; > Nit: you could just return true/false out of the if/else branches and drop Done PS2, Line 602: const shared_ptr<KsckTabletReplica> > Nit: const auto& Done http://gerrit.cloudera.org:8080/#/c/4129/2/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: PS2, Line 389: > indent Done http://gerrit.cloudera.org:8080/#/c/4129/2/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: 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 http://gerrit.cloudera.org:8080/4129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icc5196d63cbcbcbb2a9aba1ff17377b678c104bd Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
