Lars Volker has posted comments on this change. Change subject: IMPALA-3677: Write minidump on SIGUSR1 ......................................................................
Patch Set 3: (3 comments) Thank you David for the review. Let me know if those need more explanation in the 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() We use self.tmp_dir to write the minidumps to, so it should always be empty at the beginning of a test. I thought of these assertions more as a way to document what the expectations of the tests are, plus a last line of defense in case something goes wrong, or someone disables using a temporary directory. They should never fail, and if they do we can investigate why. Should I add a comment or a log message to explain this? PS3, Line 149: time.sleep(5) > Bearing in mind that I don't know really anything about how long it takes t I agree with you, this is not optimal. For killing the processes we already have a timed wait loop, and they can sometimes take a long time to terminate, especially when writing core dumps, too. Minidumps of short-lived daemons are usually around 2.5MB large, so I expect them to be written fairly quickly and would be surprised if it ever took longer than 5 seconds. I also thought about adding a wait loop here, but then I'd set it to 5 seconds still, just so we get notified in case this takes longer. Therefore I figured it's not worth the effort for now, and that we should add a loop if 5s ever becomes a problem. Do you think I should elaborate on this in a comment? PS3, Line 152: assert len(self.cluster.impalads) == 3 > Should we always assume that the number of impalads is 3? Since you saved t Done -- 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
