David Knupp has posted comments on this change. Change subject: IMPALA-3677: Write minidump on SIGUSR1 ......................................................................
Patch Set 3: (3 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 Is raising an AssertionError the right thing to do if count_all_minidumps() is not 0 at the very start of the test? Does that just mean that there are minidump artifacts left over from a previous test before this one has even started? If that's so, it doesn't seem to really be a case of SIGUSR1 failing to write minidumps. Perhaps there should be some kind cleanup process we could implement instead -- to clear out any *.dmp files before calling start_cluster()? If that were written as, say, a local test fixture or setup method, then perhaps all of the tests in this module could use it? PS3, Line 149: time.sleep(5) Bearing in mind that I don't know really anything about how long it takes to write a minidump, I'm always a bit leery when I see a hard-coded sleep in a test. It generally tends to be something of a test anti-pattern. E.g., if we ever start to run tests on a much larger cluster, would 5 still suffice? Or is 5 just such an imaginably long period time that it's guaranteed to always work? Would it make sense to move the various asserts involving count_minidumps() here -- since this is where we expect them to be created -- and instead use some kind of loop to keep checking until self.count_minidumps('impalad') returns cluster_size, setting some limit to the number of iterations before we force the test to fail? Just asking, by the way. Maybe that's more trouble than it's worth. I'm perfectly happy to accept that 5 is the right answer. PS3, Line 152: assert len(self.cluster.impalads) == 3 Should we always assume that the number of impalads is 3? Since you saved the cluster_size to a variable in an earlier line before sending the SIGUSR1, wouldn't it work here to assert that len(self.cluster.impalads) is the same after the refresh as whatever cluster_size was before? -- 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
