Francesco Romani has posted comments on this change.
Change subject: v2v: Add PipelineProc, pipeline wrapper object
......................................................................
Patch Set 2: Code-Review-1
(3 comments)
looks ok, minor comments about the API. -1 for visibility, but this is actually much
closer to +1 than a real -1.
https://gerrit.ovirt.org/#/c/62094/2/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
PS2, Line 643: _proc
_procs (please note the trailing 's') is a bit clearer
PS2, Line 648: nlike regular kill() we do not raise
: OSError if the processess have already terminated.
please add another sentence to explain why we do so.
PS2, Line 660: @property
: def pid(self):
: return [p.pid for p in self._proc]
this is supposed to mimic `pid` as returned by process wrappers returned by subprocess?
If so, the idea is good, but we return a list instead of a single integer, so we should
not have the same API if we yield different results.
Perhaps just rename "pids" or "pid_list".
--
To view, visit
https://gerrit.ovirt.org/62094
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c3741ae7ef9731a2cd9d587e86766b9e6e64f62
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes