Dan Hecht has posted comments on this change. Change subject: IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 12: (12 comments) http://gerrit.cloudera.org:8080/#/c/2028/12/be/src/common/global-flags.cc File be/src/common/global-flags.cc: Line 74: Leave "Leave" implies that's the default. Maybe "Set to empty.." http://gerrit.cloudera.org:8080/#/c/2028/12/be/src/util/minidump.cc File be/src/util/minidump.cc: Line 49: stdout and stderr Line 57: stdout stdout/stderr Line 57: INFO INFO/ERROR Line 59: snprintf technically, even snprintf() can allocate memory (and I think it does on some paths, though probably not this one). But it'd be safest to just do multiple sys_write() calls to do the string concatenation. Line 89: if (err) continue; what cases might err by non-zero? Line 111: boost::filesystem::remove(minidump_path, err); why do we use boost here but std::remove on line 97. what's the difference? Line 131: boost::filesystem::remove(to_delete->second, err); same question Line 144: assert DCHECK Line 157: create_directories FileSystemUtil::CreateDirectory() ? http://gerrit.cloudera.org:8080/#/c/2028/12/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: Line 76: time.sleep(1) can't this be flaky? Line 100: self.assert_impalad_log_contains('INFO', 'Wrote minidump to ', expected_count=1) maybe factor these out and also check 'ERROR' for the message. -- 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: 12 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
