David Knupp has posted comments on this change. Change subject: IMPALA-3677: Write minidump on SIGUSR1 ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/3312/3/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: PS3, Line 143: assert self.count_all_minidumps() == 0 > We use self.tmp_dir to write the minidumps to, so it should always be empty I think it's probably fine to leave as is -- I do see that most of the tests in this module follow this same pattern. For what it's worth, in my previous experience, I've tended to see assertions used for failures to meet the passing criteria of the test expectations, and other kinds of exceptions for setup / framework / environment errors that had nothing to do with the product under test. (e.g. the test framework I worked with in the past had its own custom exceptions library for this kind of thing.) As a general practice, this check probably is a little more defensive than necessary. Assuming the call to tempfile.mkdtemp() works, there's no reason to believe it would have anything other than 0 files in it. If there were a problem creating the temp dir, one presumes that the tempfile module would itself throw an exception before this line ever executes. PS3, Line 149: time.sleep(5) > I agree with you, this is not optimal. For killing the processes we already If it's not worth the effort, then I agree - even if it's not ideal, leave it as is until it poses a problem. -- To view, visit http://gerrit.cloudera.org:8080/3312 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40149e48e391451de21a5c8bda18e2307fc89513 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
