Mike Percy has posted comments on this change. Change subject: Add Google Breakpad support to Kudu ......................................................................
Patch Set 6: (21 comments) 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 writt Done 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? The first one never left; the second one is back in 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. It's possible, but I didn't do the breakpad build work to make it happen. We'd have to figure out how to make the breakpad client library build on Mac. They only provide Xcode files to build it; the Makefiles specifically exclude Mac and making breakpad work on Mac seems very low priority-wise. 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 I think this is more discoverable if someone wants to come around later and add MacOS breakpad support. But yeah, the other way is a little neater. I don't have a strong preference so I'll take your suggestion. Line 68: master->process()->Kill(SIGSEGV); > Maybe run the tserver/master test in a loop and try each signal for which y I do something like that in a different test. Line 72: TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) { > I thought this functionality was removed? I don't think it needs to be removed now. But I posted this patch revision before I tried to fix it. 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. good catch PS6, Line 326: // Same with minidumps. > Nit: could you work this into the previous comment instead? Done PS6, Line 329: excess-glog-deleter" > Should generalize the thread name. Done PS6, Line 330: excess_glog_deleter_thread_ > Probably rename this too. Done 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 Disabled building this file on MacOS. Line 67: CHECK_EQ(1, 0); > Maybe just invoke the FAIL() macro? Not sure that would be fatal but I switched this to abort(). Line 78: kill(getpid(), SIGUSR1); > I thought you removed the SIGUSR1 functionality? See my other comments 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 m It sounds good in theory but then you have to worry about gflag DEFINEs (do they just not get defined on Mac or do we have to put them in the stub file too?) and we end up with extra build stuff. Having #ifdefs in here doesn't seem that bad to me. Line 66: LOG(ERROR) << "Minidumps are not supported on this platform"; > Is it actually safe to LOG(ERROR) in this context? Has glog been initialize It should just go to stderr. But I'll simply remove this message anyway. Line 98: DEFINE_bool(minidump_invoke_previous_signal_handler, true, > What's the motivation for this? Why ever set it to false? I was originally unsure whether this would work. I don't think we need to keep the flag, so I'll remove it. 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); For consistency: To avoid a signal being either safe to send with one command line flag set or fatal with another command line flag set. It's better to make it so that it's always safe to send the signal to the process. The way it's written, in one case it's a no-op and in the case it generates a minidump. Line 337: if (max_minidumps <= 0) return Status::OK(); > Nit: format this simple if/return the way the FLAGS_enable_minidumps one is Done 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? See elsewhere Line 41: // Only one instance of this class may be instantiated at a time. > Having reviewed the singleton-based approach, it seems more complicated tha I attempted using a GoogleOnce-based approach before rewriting this as a singleton. It was painful for the following reasons: 1. I wanted to put the minidump files in a location that would be cleaned up after each test. In order to do that in a test case I would have to manage the creation / cleanup in the test case itself. 2. Without non-gflag persistent state to rely on it became difficult to know / remember where we were writing our minidump files to across test invocations. It was much cleaner to have some object state independent of gflags. Using a singleton results in more intuitive and predictable test code than what we were doing before, which was modifying gflags to keep track of the location of the minidump directory and then reading it back later. I went a step further and used a singleton with a lifecycle so that I wouldn't have to manage cleaning up the minidump directory between tests and bringing up the singleton at the test case level instead of the test level. It didn't seem to add much additional complexity to achieve that. It's possible to roll back some of the lifecycle and make it a "live forever" singleton but I think it's pretty clean as written now. 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? Done -- 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 <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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
