Lars Volker has posted comments on this change. Change subject: PREVIEW IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 5: (7 comments) Thanks for the review. Please see PS6. 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. Done 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. Done. Was some leftover from a previous iteration, which I forgot to clean up. :( Line 52: using namespace boost::filesystem; > Same here, not needed? Done Line 144: impala::CheckAndRemoveMinidumps(FLAGS_max_minidumps); > Maybe move this to a startup task instead of recurring? After the first cal Very good point, done. 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? Done Line 72: if (bf::is_regular_file(minidump_path)) { > This looks like an exception throwing signature. Looks like there are a cou Done Line 75: time_t last_written = bf::last_write_time(minidump_path); > Same with this signature. Maybe always use the error code verison? Otherwis 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: 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
