[kudu-CR] KUDU-2353 (part 1): add a tool to parse stacks from diagnostics log

["Will Berkeley (Code Review)" <ge...@cloudera.org>]

Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9773 )

Change subject: KUDU-2353 (part 1): add a tool to parse stacks from diagnostics log
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/9773/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9773/1//COMMIT_MSG@9
PS1, Line 9: s
> nit: extra s
Done


http://gerrit.cloudera.org:8080/#/c/9773/1//COMMIT_MSG@13
PS1, Line 13: parse-stacks
To be consistent with existing tools, the name should be parse_stacks.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/gutil/strings/split_internal.h
File src/kudu/gutil/strings/split_internal.h:

http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/gutil/strings/split_internal.h@28
PS1, Line 28: base
Seems this should be kudu/gutil, not base.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/gutil/strings/split_internal.h@260
PS1, Line 260:   operator std::array<ArrayType, ArraySize>() {
> warning: 'operator array<type-parameter-1-0, ArraySize>' must be marked exp
I actually get a compile error if I follow Tidy Bot's advice here.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/kudu-tool-test.cc@2417
PS1, Line 2417: "t
> nit: Indentation off by a tad.
Done


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_action_diagnose.cc
File src/kudu/tools/tool_action_diagnose.cc:

http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_action_diagnose.cc@53
PS1, Line 53: using std::cerr;
> warning: using decl 'cerr' is unused [misc-unused-using-decls]
Again, compile error if I comply.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_action_diagnose.cc@134
PS1, Line 134:   const string kUnknown = "<unknown>";
> warning: invalid case style for private member 'kUnknown' [readability-iden
Bit of a special case to avoid a temporary. Ignoring.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_action_diagnose.cc@248
PS1, Line 248: StacksRecord::Group* group
Should DCHECK_NOTNULL this argument.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_action_diagnose.cc@289
PS1, Line 289: StacksRecord* record)
Likewise.


http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:

http://gerrit.cloudera.org:8080/#/c/9773/1/src/kudu/tools/tool_main.cc@65
PS1, Line 65:       .AddMode(BuildDiagnoseMode())
> Please also update the "help" test in kudu-tool-test.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/9773
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5969cb3a54f691356e9cd3add150e717538a687
Gerrit-Change-Number: 9773
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 May 2018 06:35:48 +0000
Gerrit-HasComments: Yes