Adar Dembo 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 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/kudu-tool-test.cc@2821 PS2, Line 2821: &stdout));; Nit: extra semicolon here. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc File src/kudu/tools/tool_action_diagnose.cc: http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@105 PS2, Line 105: InsertIfNotPresent(&symbols_, addr, symbol); If addr/symbol aren't moved, VisitSymbol should receive them by cref. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@112 PS2, Line 112: first_ = false; Nit: could move to just past L110. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@143 PS2, Line 143: Status Parse(const std::string& line) { Nit: don't need std:: prefix. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@155 PS2, Line 155: date_ = fields[0].ToString(); Nit: stylistically would prefer if Parse() didn't mutate its internal state if returning an error. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@164 PS2, Line 164: int64_t time_us; : if (!safe_strto64(fields[3].data(), fields[3].size(), &time_us)) { : return Status::InvalidArgument("invalid timestamp", fields[3].ToString()); : } Why bother parsing this if we're not going to use it? Just as a sanity check? http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@188 PS2, Line 188: string date_; : string time_; Seems like you could avoid making these copies if the ParsedLine took ownership of 'line' in Parse() and stored StringPieces here (which are just pointers into other strings, basically). http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@211 PS2, Line 211: ParsedLine lf; Nit: Not sure what 'lf' means here. Maybe that was from a time when ParsedLine was called something else? http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@219 PS2, Line 219: RETURN_NOT_OK(ParseStacks(lf, &stacks_record)); For symmetry, could we make ParseStacks a non-static function and have it be responsible for invoking VisitStacksRecord? http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@231 PS2, Line 231: CHECK(lf.type() == RecordType::kSymbols); Can we use CHECK_EQ instead? Guessing that didn't work because there's no operator<< defined for RecordType. You could implement a simple one. See threadpool.{cc,h} for an example. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@335 PS2, Line 335: return Status::IOError(ErrnoToString(errno)); Does errno work in this context? Is it guaranteed to be set by the C++ stream library? -- 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: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 23 May 2018 04:32:55 +0000 Gerrit-HasComments: Yes
