Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12501 )
Change subject: IMPALA-8191: Wait for additional breakpad processes during test ...................................................................... Patch Set 1: (3 comments) Ok. Thanks for explaining. I'm fine with the tactical fix, but I think doing process.get_pids() is probably easy enough and will be less confusing. http://gerrit.cloudera.org:8080/#/c/12501/1/tests/common/impala_cluster.py File tests/common/impala_cluster.py: http://gerrit.cloudera.org:8080/#/c/12501/1/tests/common/impala_cluster.py@321 PS1, Line 321: if set(self.cmd) == set(process.cmdline): Do you know why this is set? http://gerrit.cloudera.org:8080/#/c/12501/1/tests/common/impala_cluster.py@322 PS1, Line 322: return pid Instead of returning pid here, you could collect all matches, and then either pick the better one (parent) or fail? Would that have made this more obvious? http://gerrit.cloudera.org:8080/#/c/12501/1/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: http://gerrit.cloudera.org:8080/#/c/12501/1/tests/custom_cluster/test_breakpad.py@88 PS1, Line 88: def wait_for_all_processes_dead(self, processes, timeout=300): : # For every process in the list we might see the original Impala process plus a forked : # off child that is writing the minidump. To catch both we go through the list twice. : for process in processes * 2: > These are Process objects (https://github.com/apache/impala/blob/master/tes OMG. I would never have thought that "Process" is really "output of pgrep". How would you feel about changing this to process.get_pids() here, so that it's more obvious that a process can get represented multiple times in this case, and we have an answer? -- To view, visit http://gerrit.cloudera.org:8080/12501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4dcc5fecb9b5f38ae1504aae40f099837cf1bca Gerrit-Change-Number: 12501 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Sat, 16 Feb 2019 00:27:02 +0000 Gerrit-HasComments: Yes
