Todd Lipcon has posted comments on this change. Change subject: Add Google Breakpad support to Kudu ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/5620/1//COMMIT_MSG Commit Message: Line 28: because breakpad does not properly namespace its includes (however it I'm not sure what you mean by "namespace" here. You don't mean namespace as in C++ namespace, but rather as in a directory prefix? In that case, it seems odd to me to put them in the $PREFIX/include dir at all, because people are going to be confused how the includes work. Alternatively, maybe it would be preferably to post-process the installed directory? ie install into the normal prefix and then run something like: find $PREFIX/include/breakpad -type f | xargs perl -p -i -e 's,^#include "(?!breakpad/),$&breakpad/,' from a quick inspection of the source it appears like they are self-consistent about using #include "..." for breakpad headers and #include <...> for system headers. http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/integration-tests/minidump_generation-itest.cc File src/kudu/integration-tests/minidump_generation-itest.cc: Line 56: ASSERT_TRUE(found) << "Unable to find minidump after " << timeout.ToString(); could you restructure this test using AssertEventually(...) to be a bit easier to read? eg pseudocode: AssertEventually([&](){ vector<string> matches; ASSERT_OK(env_->Glob(dir + "/*.dmp", &matches)); ASSERT_EQ(1, matches.size()); }); http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: Line 208: breakpad_client does putting this stuff in util/ mean that we're ending up linking unnecessary junk into our client library? or does the linking process end up not including any of the breakpad stuff because client code never calls it? http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: Line 99: const int path_len = my_strlen(path); I don't think it's calling into libc that's problematic, is it? I think the real issue is async signal safety of strlen(), though according to http://austin-group-l.opengroup.narkive.com/jBp07fPN/adding-simple-string-functions-to-async-signal-safe-list it is in practice and seems like it has been designated as such in later versions of the standard). I'm also surprised to see 'my_strlen' is defined by breakpad without any namespacing or anything. yuck. PS1, Line 220: CreateDirsRecursively we sure that recursive create is what we want vs just creating the parent and the daemon directory? http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.h File src/kudu/util/minidump.h: Line 26: Status RegisterMinidump(const char* cmd_line_path); seems odd to see a C string here instead of std::string -- To view, visit http://gerrit.cloudera.org:8080/5620 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
