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 2: (11 comments) Didn't add any extra test for the JSON visiting yet. 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. Done 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. Done 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. I'm not following. That would result in first_ being true always. 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. Done 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 Done 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 chec Right now it's a sanity check but it should be used later when we parse and do things with the metrics that are also in the log. I added a note to this effect. 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 owner That's true, though we'd still make a copy of the JSON payload unless the TODO is addressed. Do you think it's worth it? My reservation is that it goes against my intuition for a parser to take ownership of part of the stream it is parsing, and there's no need for the extra efficiency here AFAICT. 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 ParsedL Done 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 b Done 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 o Done 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++ stre After a bit of searching, it's seems like probably it's not guaranteed to be set by the C++ standard, but on Linux and macos we can count on it being set. -- 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 21:03:31 +0000 Gerrit-HasComments: Yes
