Adar Dembo has posted comments on this change. Change subject: Add Google Breakpad support to Kudu ......................................................................
Patch Set 6: (22 comments) > The right way to do it would probably be to have a signal-handling thread. Another option is to write a minidump in response to a krpc request. That means it could be requested remotely, which is both good and bad. http://gerrit.cloudera.org:8080/#/c/5620/6//COMMIT_MSG Commit Message: PS6, Line 29: default Nit: probably don't need 'default' here (since the minidump files are written to whatever the log directory is, regardless of whether it's the default value or not). PS6, Line 51: * By default, invoke previously-installed signal handler if installed. : * Install SIGUSR1 handler to create minidump on demand. These two aren't in any more, right? http://gerrit.cloudera.org:8080/#/c/5620/6/CMakeLists.txt File CMakeLists.txt: Line 915: if (NOT APPLE) Oh, I thought it did work on macOS. Bummer. http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/integration-tests/minidump_generation-itest.cc File src/kudu/integration-tests/minidump_generation-itest.cc: Line 52: #if !defined(__linux__) Wouldn't it be cleaner to just not build the test for macOS? That is, patch CMakeLists.txt instead of #ifdef here? Line 68: master->process()->Kill(SIGSEGV); Maybe run the tserver/master test in a loop and try each signal for which you expect breakpad to generate a dump? Line 72: TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) { I thought this functionality was removed? http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: PS6, Line 318: if (FLAGS_logtostderr) { : return Status::OK(); : } This is no longer correct. PS6, Line 326: // Same with minidumps. Nit: could you work this into the previous comment instead? PS6, Line 329: excess-glog-deleter" Should generalize the thread name. PS6, Line 330: excess_glog_deleter_thread_ Probably rename this too. http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump-test.cc File src/kudu/util/minidump-test.cc: PS6, Line 60: #if !defined(__linux__) : return; : #endif Again, I think we should just not build the test on macOS. Also, shouldn't the second test be conditioned? Line 67: CHECK_EQ(1, 0); Maybe just invoke the FAIL() macro? Line 78: kill(getpid(), SIGUSR1); I thought you removed the SIGUSR1 functionality? http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: Line 118: // Write two null-terminated strings and a newline to both stdout and stderr. > Good catch! Looking at the implementation, WriteMinidump() calls memset(), I left a bunch of comments to the effect of "but I thought we were removing the SIGUSR1 functionality?" after forgetting that you intended to remove it but haven't yet. You can ignore them. http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.cc File src/kudu/util/minidump.cc: PS6, Line 28: #if defined(__linux__) : #include <breakpad/client/linux/handler/exception_handler.h> : #include <breakpad/common/linux/linux_libc_support.h> : #endif // defined(__linux__) How about just not compiling minidump.cc for macOS? Or providing a second minidump_stubs.cc file that defines the public methods to no-op? Line 66: LOG(ERROR) << "Minidumps are not supported on this platform"; Is it actually safe to LOG(ERROR) in this context? Has glog been initialized by this point? Line 98: DEFINE_bool(minidump_invoke_previous_signal_handler, true, What's the motivation for this? Why ever set it to false? PS6, Line 264: // Under Linux we always set up a signal handler for SIGUSR1. This is so that : // when minidumps are not enabled, we will not crash, we will simply no-op. I don't understand the motivation (though again, maybe it's a moot point); if minidumping is disabled, why shouldn't SIGUSR1 kill the process as it normally does? Line 337: if (max_minidumps <= 0) return Status::OK(); Nit: format this simple if/return the way the FLAGS_enable_minidumps one is formatted? Or the other way around? http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.h File src/kudu/util/minidump.h: PS6, Line 35: r if it : // received a USR1 signal I thought you removed this? Line 41: // Only one instance of this class may be instantiated at a time. Having reviewed the singleton-based approach, it seems more complicated than the previous one. How exactly does it simplify testing? Seems like encapsulation could have been achieved previously with a test fixture whose constructor initialized minidump writing and whose destructor uninitialized it. If you choose to continue down the singleton path, can some of this logic be simplified with gutil/once.h or util/once.h? PS6, Line 56: Should not be called from a crash context : // because it uses the heap. Should not be called from a signal handler either, right? -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
