Casey Ching has posted comments on this change. Change subject: PREVIEW IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/2028/5/be/src/common/global-flags.cc File be/src/common/global-flags.cc: Line 70: DEFINE_string(minidump_path, "/tmp/minidumps", "Directory to write minidump files to. " Maybe add something about "impala" to the path. Also it would be nice to have short description of what a minidump and why it is useful. http://gerrit.cloudera.org:8080/#/c/2028/5/be/src/common/init.cc File be/src/common/init.cc: Line 17: #include <boost/filesystem.hpp> Remove? I don't see where this is used. Line 52: using namespace boost::filesystem; Same here, not needed? Line 144: impala::CheckAndRemoveMinidumps(FLAGS_max_minidumps); Maybe move this to a startup task instead of recurring? After the first call there should only be something to remove if the process crashes right? http://gerrit.cloudera.org:8080/#/c/2028/5/be/src/util/minidump.cc File be/src/util/minidump.cc: Line 67: "??????\?\?-??\?\?-??\?\?-??????\?\?-????????.dmp"; Can you give an example of what this matches? Line 72: if (bf::is_regular_file(minidump_path)) { This looks like an exception throwing signature. Looks like there are a couple others that don't throw. http://www.boost.org/doc/libs/1_57_0/libs/filesystem/doc/reference.html#is_regular_file Line 75: time_t last_written = bf::last_write_time(minidump_path); Same with this signature. Maybe always use the error code verison? Otherwise there should be some exception handling. From what I heard, Impala prefers not to avoid exceptions though. Can you check any other functions too? -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-HasComments: Yes
