Lars Volker has posted comments on this change. Change subject: PREVIEW IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 3: (9 comments) Thanks Casey for the review. Can you have another look? http://gerrit.cloudera.org:8080/#/c/2028/3/be/src/util/minidump.cc File be/src/util/minidump.cc: Line 1: // Copyright 2012 Cloudera Inc. > Update year Done Line 42: tm* now = gmtime(&now_epoch); > Probably better to use the threadsafe version http://linux.die.net/man/3/gm Code removed altogether. Line 54: string filename = MINIDUMP_PREFIX + "." + GetTimePidString() + ".dmp"; > How about moving "dmp" to a constant since its used below too? Code removed altogether. Line 59: printf("Error writing minidump to: %s (error was: %s)\n", new_path.c_str(), > Any idea where the user sees this output? Depending how the impala daemon is started it would end up with the rest of stdout. In our case it ends up in the .INFO files. I noticed that for statestored an additional flush() is necessary. Any idea why? Line 66: BOOST_UNREACHABLE_RETURN( false ); > Why is this needed? I'm used to using it as some compilers complain about unreachable code otherwise. Code removed altogether. Line 69: static bool DumpCallback(const google_breakpad::MinidumpDescriptor& descriptor, > The docs at https://chromium.googlesource.com/breakpad/breakpad/+/master/do I changed the approach now to not do anything when writing a minidump. Instead the periodic cleanup thread inspects the minidump files and extracts the timestamp from them. Line 73: return succeeded; > So what happens if this return value is true? Does the program still die? Yes it will, it only affects whether further handlers are called. https://chromium.googlesource.com/breakpad/breakpad/+/master/src/client/linux/handler/exception_handler.h#93 I added a comment. Line 124: (void)eh; > Just curious, what does this do? It prevents the compiler from complaining about the unused variable. http://gerrit.cloudera.org:8080/#/c/2028/3/be/src/util/minidump.h File be/src/util/minidump.h: Line 1: // Copyright 2012 Cloudera Inc. > Update year Done -- To view, visit http://gerrit.cloudera.org:8080/2028 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7a37a38488716ffe34296f3490ae291bbb7228d6 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Casey Ching <ca...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Silvius Rus <s...@cloudera.com> Gerrit-HasComments: Yes
