Yaniv Bronhaim has posted comments on this change. Change subject: Introducing context manager to terminate async processes on internal fail ......................................................................
Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/51407/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 704: Line 705: def terminator(proc, logger): Line 706: ''' Line 707: A context manager for taking care for killing process on failure. Line 708: The first exception will be remembered and re-raised. > I think this should always terminate the process, not only after errors, li but in most of the places you just want to kill on error, and let the process continue, or wait until it finishes this decorator just guaranty that we kill the process on fail. we could do it inside asyncProc for each method, i think its more reasonable Line 709: Line 710: Sample usage: Line 711: proc = ExecCmd('cmd', sync=False) Line 712: with terminator(proc, logger) as rollback: Line 733: try: Line 734: proc.kill() Line 735: except Exception: Line 736: self._log.error("Failed to kill subprocess") Line 737: six.reraise(*exc_info) > It will be much simpler to use a function instead of a class with __enter__ I don't think I understand what you suggest. where do you plan to use such contextmanager? Line 738: Line 739: Line 740: def traceback(on="", msg="Unhandled exception"): Line 741: """ -- To view, visit https://gerrit.ovirt.org/51407 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I003cce39d62dba5644937fafaf2c6e24c526c075 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
