Tim Armstrong has posted comments on this change. Change subject: IMPALA-2686: Add breakpad crash handler to all daemons ......................................................................
Patch Set 9: (10 comments) Looking good, just had a bunch of fairly minor comments. 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 'filesystem' or 'system' or just done 'using boost::package::function' for the functions we're actually using. It would be good to keep it consistent. Line 129: to_delete nit: we'd typically putthis in the for() list to make clear it's a loop variable. I.e. for(int i = 0; i < files_to_delete, ++i, ++to_delete) Line 149: bf::create_directories(FLAGS_minidump_path, err); We already have wrappers for some of these functions in filesystem-util.cc that convert error codes to Status and handle various things. E.g. there is a bug in some routines that can throw exceptions even when not using the exception interface. I think it would be good to use CreateDirectory() here Line 158: bf::permissions(FLAGS_minidump_path, bf::owner_all, err); Consider making this a utility function in FilesystemUtil to restrict permissions. Also, does owner_all give execute permissions? It seems like we only want rw. 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 save some compile cycles. Line 25: #define IMPALA_QUERY_LOG_NUM_ENTRIES 128 We usually implement constants as class members. Line 31: void RegisterMinidump(const char* cmd_line_path); Currently this terminates the process if it fails. This should be documented in the header. Alternatively, you could return a status and let the caller handle it. 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 native-toolchain before but it seems like it we kind of need it to have a consistent breakpad version. Do you see any alternative to this that would let someone build without the toolchain? I'm not opposed to going forward like this but it doesn't seem ideal. http://gerrit.cloudera.org:8080/#/c/2028/9/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: Line 30: """Check that breakpad integration into the daemons works as expected. This includes This is great, thanks for doing this. Line 81: def test_minidump_creation(self): I think we should make all these tests exhaustive only. The coverage is valuable but I think it would be rare that someone would check in code that would break it. -- 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
