impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-3677: Write minidump on SIGUSR1
Date Thu, 07 Jul 2016 20:22:21 GMT
David Knupp has posted comments on this change.

Change subject: IMPALA-3677: Write minidump on SIGUSR1

Patch Set 3:

File tests/custom_cluster/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I40149e48e391451de21a5c8bda18e2307fc89513
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message