Change in vdsm[master]: virt: introduce "Async" helper
fromani at redhat.com
fromani at redhat.com
Fri Nov 27 10:11:58 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: virt: introduce "Async" helper
......................................................................
Patch Set 2:
(11 comments)
The issue I'm trying the address is the split logic introduced in
https://gerrit.ovirt.org/#/c/45954/19 and parent patch.
I'm still not comfortable with that approach, somehow feels fragile.
I fully concur that Async() has little-to none sense. AsyncThrottler IMO has, and its purpose is to incapsulate the split incoming acquire/release logic.
Async() serves the only purpose to provide common API in vm.py async.do(...)
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?
Not really. Will compact up things.
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()'
Good point. Let me add this.
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 -- se
Yes, I was unsure myself. Better just expose do(), and keep this code private.
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.
Done
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?
I need to run some pre-startup checks in the thread we attempt to run. Here there is little point, but please check AsyncThrottler.
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 no
Good point. Fixed.
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 ...
Thanks, fixed
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 ...
Thanks, fixed
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 ...
Tyanks, fixed
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.
Indeed. Fixed.
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:
Sounds good, let's see how it looks.
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 <fromani at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list