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 3: (3 comments) 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@112 PS2, Line 112: }; > I'm not following. That would result in first_ being true always. Brain fart; I didn't see the negation on the condition in L109. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@188 PS2, Line 188: } else { : type_ = R > That's true, though we'd still make a copy of the JSON payload unless the T Well, LogParser won't be taking ownership of part of the stream; ParsedLine will, which sorta makes sense (since it's taking ownership over "its" line). As for efficiency, how large are the metrics logs? If they're big, or if the tool is expected to be used against many of them, extra efficiency would be appreciated. http://gerrit.cloudera.org:8080/#/c/9773/2/src/kudu/tools/tool_action_diagnose.cc@335 PS2, Line 335: ++group) { > After a bit of searching, it's seems like probably it's not guaranteed to b Can you show me how you reached that conclusion? Not saying you're wrong; I just want to better understand myself. -- 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: 3 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:53:08 +0000 Gerrit-HasComments: Yes
