Alexey Serbin has posted comments on this change. Change subject: Add Google Breakpad support to Kudu ......................................................................
Patch Set 3: (10 comments) Does it work on OS X? Consider adding corresponding ifdefs at least at the first pass to allow this to compile on OS X. http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/integration-tests/minidump_generation-itest.cc File src/kudu/integration-tests/minidump_generation-itest.cc: PS3, Line 21: #include <glog/logging.h> nit: is it needed? Line 21: #include <glog/logging.h> nit: consider adding <gtest/gtest.h> PS3, Line 22: #include <glog/stl_logging.h> nit: is it needed? PS3, Line 24: #include "kudu/gutil/strings/util.h" nit: is it needed? PS3, Line 26: #include "kudu/util/logging.h" nit: is it needed? Line 28: #include "kudu/util/subprocess.h" nit: consider including "kudu/util/monotime.h" (for MonoDelta) http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/master/master_main.cc File src/kudu/master/master_main.cc: PS3, Line 60: argv[0] nit: consider adding a var for argv[0] and using it around instead. http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: PS3, Line 57: Set to empty to " : "disable writing minidump files. I see there is also 'enable_minidumps' flag which is the explicit control flag for this behavior. From the usability point, I think it's better to have only one flag controlling minidumps generation, if possible. Would be that a problem generating/writing minidumps when this property were effectively empty? http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: PS3, Line 284: F_ALL What is F_ALL? I.e., where is it defined and why does it affect only the breakpad component? http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: Line 244: nit: an extra-line, seems like a random change -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
