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