Lars Volker has posted comments on this change. Change subject: IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 9: (12 comments) Thanks for the review. Please see PS10. http://gerrit.cloudera.org:8080/#/c/2028/9/be/src/util/minidump.cc File be/src/util/minidump.cc: Line 33: namespace bf = boost::filesystem; > Nit: elsewhere in the codeback we've either abbreviated these as 'filesyste Done Line 122: remove filesystem-util.h has RemovePaths() which looks like it's built for directories. Removing files with remove() here doesn't seem to be a problem. Let me know if I should change FilesystemUtil to also remove single files and use that here. Log file rotation calls unlink(), which would also be an option here. Line 129: to_delete > nit: we'd typically putthis in the for() list to make clear it's a loop var Done Line 149: bf::create_directories(FLAGS_minidump_path, err); > We already have wrappers for some of these functions in filesystem-util.cc If the directory already exists, CreateDirectory() removes it before re-creating it. The part that does the removal is also what can throw. In this case here we need to preserve the directory if it already exists. Should I change CreateDirectory() and add a flag for that? Or just go with create_directories, which is what CreateDirectory() does as well? Line 158: bf::permissions(FLAGS_minidump_path, bf::owner_all, err); > Consider making this a utility function in FilesystemUtil to restrict permi I tried that, although I was not really happy with the interface. boost::filesystem::permissions takes an enum and can add/remove/set all permission bits. I neither wanted to introduce a dependency to boost::filesystem::perms to filesystem-util.h, nor replicate the full functionality with own enums. As breakpad set permission to 600 on minidump files already, the most we could give away is the fact that crashes have happened. Hence I removed the code altogether. If you think we should keep it, I'd prefer the old way though, as it seemed more clear to me. http://gerrit.cloudera.org:8080/#/c/2028/9/be/src/util/minidump.h File be/src/util/minidump.h: Line 22: #include <boost/cstdint.hpp> > These aren't needed in the header, so lets put them in the .cc file and sav Done Line 25: #define IMPALA_QUERY_LOG_NUM_ENTRIES 128 > We usually implement constants as class members. Done Line 31: void RegisterMinidump(const char* cmd_line_path); > Currently this terminates the process if it fails. This should be documente Done http://gerrit.cloudera.org:8080/#/c/2028/9/bin/impala-config.sh File bin/impala-config.sh: Line 231: export IMPALA_BREAKPAD_VERSION=20150612-p1 > I guess breakpad doesn't have normal releases. In theory we didn't mandate I don't see an alternative, short of finding a way to completely remove breakpad support from impalad by ifdef'ing it away, which I'm not sure will be worth the effort. I'm open for suggestions, but can't think of anything easier myself. http://gerrit.cloudera.org:8080/#/c/2028/9/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: Line 50: def teardown_class(cls): > Doesn't this need a "@classmethod"? Yes, fixed it. I think it worked before because it's inherited and the base class has the decorator. Line 81: def test_minidump_creation(self): > I think we should make all these tests exhaustive only. The coverage is val Done Line 82: """Check that when a daemon crashes it writes a minidump file.""" > Checking that a core dump is created and that all the processes are dead wo I added a check to make sure all processes are dead. Do you have a suggestion how to find the relevant core dump file? It looks like different systems will handle/create it differently and I the only way I can think of is to parse /proc/sys/kernel/core_pattern. Then I would need to record the working directories of all daemons from /proc/(pid)/cwd before I kill them so I can retrieve the files later. -- 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: 9 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
