Milan Zamazal has posted comments on this change. Change subject: virt: introduce "Async" helper ......................................................................
Patch Set 2: Code-Review-1 (11 comments) I can't get rid of feeling that the implementation is too complicated for the simple purpose of checking the start success. Maybe using a semaphore inside do() and _set_started() would be a bit easier, not sure, already confused now :-). Or perhaps I completely miss something. https://gerrit.ovirt.org/#/c/49114/2/tests/vmUtilsTests.py File tests/vmUtilsTests.py: Line 195: done.set() Line 196: Line 197: self.async.do(helper) Line 198: Line 199: self.assertTrue(done.wait(1.)) Too many blank lines in all the tests? Are they needed at all? Line 200: Line 201: def test_raises_if_busy(self): Line 202: Line 203: def helper(): https://gerrit.ovirt.org/#/c/49114/2/vdsm/virt/utils.py File vdsm/virt/utils.py: Line 133: Line 134: Async takes care only of the startup phase; Line 135: it doesn't report or handle any error that Line 136: may happen in the callable right after the startup. Line 137: """ It should be documented here that the instance can't be reused, i.e. `do()' shouldn't be called twice (it'd be even better if it was additionally prevented or perhaps the instance made reusable). Line 138: Line 139: def __init__(self): Line 140: self._func = None Line 141: self._started = False Line 143: self._starting = threading.Condition() Line 144: Line 145: @property Line 146: def error(self): Line 147: """ Why property? It's quite confusing to have self._error and self.error -- self._error and self.error() would be better. And do we need this public method at all? do() already provides the information in some way. So unless we want to examine the result in a separate thread (really?), this public method is not needed, as demonstrated in vm.py. Line 148: Returns the startup error of the callable, or Line 149: None if operation succeded. Line 150: Block until the callable started, either succesfully Line 151: or failing. Line 162: `func' may be callable without arguments. Line 163: The return value of `func' is ignored. Line 164: `daemon' and `logger' arguments behave like Line 165: concurrent.thread. Line 166: """ Please document that the method throws an exception on start error. Line 167: Line 168: self._func = func Line 169: self._thread = concurrent.thread( Line 170: self._run, Line 164: `daemon' and `logger' arguments behave like Line 165: concurrent.thread. Line 166: """ Line 167: Line 168: self._func = func Why using the attribute and not pass `func' to concurrent.thread? Line 169: self._thread = concurrent.thread( Line 170: self._run, Line 171: daemon=daemon, Line 172: logger=logger) Line 172: logger=logger) Line 173: self._thread.start() Line 174: Line 175: err = self.error Line 176: if err: According to error() docstring anything but None is error, so `if err is not None:' should be used here. Line 177: raise AsyncStartError(err) Line 178: Line 179: def _run(self): Line 180: self._set_started(error=None) Line 189: Line 190: class AsyncThrottler(Async): Line 191: """ Line 192: AsyncThrottler runs a callable into a background thread, Line 193: like Async, but only if can acquire an user-provided ... if it can acquire a ... Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) Line 197: if it can't acquire such semaphore. Line 192: AsyncThrottler runs a callable into a background thread, Line 193: like Async, but only if can acquire an user-provided Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) ... do() raises AsyncStartError ... Line 197: if it can't acquire such semaphore. Line 198: In that case returns the user-provided error code. Line 199: """ Line 200: Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) Line 197: if it can't acquire such semaphore. Line 198: In that case returns the user-provided error code. ... case it returns ... Line 199: """ Line 200: Line 201: def __init__(self, sem, errcode): Line 202: super(AsyncThrottler, self).__init__() Line 201: def __init__(self, sem, errcode): Line 202: super(AsyncThrottler, self).__init__() Line 203: self._sem = sem Line 204: self._errcode = errcode Line 205: _error_code and error_code would be better names. Line 206: def _run(self): Line 207: acquired = self._sem.acquire(False) Line 208: if not acquired: Line 209: self._set_started(error=self._errcode) Line 204: self._errcode = errcode Line 205: Line 206: def _run(self): Line 207: acquired = self._sem.acquire(False) Line 208: if not acquired: I'd prefer `if acquired: ... else: ...' for two reasons: 1. `not' in if-part is ugly. 2. success is typically handled before failure (see `try'). Line 209: self._set_started(error=self._errcode) Line 210: else: Line 211: self._set_started(error=None) Line 212: try: -- To view, visit https://gerrit.ovirt.org/49114 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd277197c7c3819008ae71a8cf2c0679b901397e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@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
