Change in vdsm[master]: executor: introduce the executor library
danken at redhat.com
danken at redhat.com
Tue Jan 20 16:25:36 UTC 2015
Dan Kenigsberg has posted comments on this change.
Change subject: executor: introduce the executor library
......................................................................
Patch Set 31: Code-Review-1
(7 comments)
http://gerrit.ovirt.org/#/c/29191/31/lib/vdsm/executor.py
File lib/vdsm/executor.py:
Line 68: self._log.debug('Stopping executor')
Line 69: with self._lock:
Line 70: self._running = False
Line 71: self._tasks.clear()
Line 72: for i in range(self._workers_count):
i -> _
Line 73: self._tasks.put((_STOP, None))
Line 74: workers = tuple(self._workers) if wait else ()
Line 75: for worker in workers:
Line 76: try:
Line 82: """dispatches a new task to the executor.
Line 83:
Line 84: The task may be any callable.
Line 85: The task will be executed as soon as possible
Line 86: in one of the active workers of the executor."""
please document that timeout is measured since callable is called.
Line 87: if not self._running:
Line 88: raise NotRunning()
Line 89: self._tasks.put((callable, timeout))
Line 90:
Line 144: self._scheduler = scheduler
Line 145: self._busy = False
Line 146: self._discarded = False
Line 147: _Worker._id += 1
Line 148: name = "Executor-%i" % _Worker._id
if we ever want to have more than one executor, we would need to do a similar trick to enumerate them.
I personally find name = Worker-%i less surprising.
Line 149: self._thread = threading.Thread(target=self._run, name=name)
Line 150: self._thread.daemon = True
Line 151: self._log.debug('Starting worker %s' % name)
Line 152: self._thread.start()
Line 154: @property
Line 155: def name(self):
Line 156: return self._thread.name
Line 157:
Line 158: def join(self):
we attempt to join only the non-busy workers. why? why should we join() at all?
Line 159: if self._busy:
Line 160: raise _WorkerBusy()
Line 161: self._log.debug('Waiting for worker %s', self.name)
Line 162: self._thread.join()
Line 155: def name(self):
Line 156: return self._thread.name
Line 157:
Line 158: def join(self):
Line 159: if self._busy:
race: if join() is called just before _busy was set in _executre_task we can end up joining a busy task.
Line 160: raise _WorkerBusy()
Line 161: self._log.debug('Waiting for worker %s', self.name)
Line 162: self._thread.join()
Line 163:
Line 215: # https://docs.python.org/2.6/library/queue.html#Queue.Full
Line 216: self._cond = threading.Condition(threading.Lock())
Line 217: self._waiters = 0
Line 218:
Line 219: def put(self, task):
add a comment
# we don't really mind the race here. in the worse case, we may have
# a bit more than _max_tasks.
Line 220: if len(self._tasks) == self._max_tasks:
Line 221: raise TooManyTasks()
Line 222: self._tasks.append(task)
Line 223: if self._waiters > 0:
Line 231: except IndexError:
Line 232: with self._cond:
Line 233: self._waiters += 1
Line 234: if not self._tasks:
Line 235: self._cond.wait()
there's a race here if multiple waiting threads have been woken.
can we require pthreading without the spurious wakeups bug, and drop _waiters? otherwise, use a semaphore to count waiters.
Line 236: self._waiters -= 1
Line 237:
Line 238: def clear(self):
--
To view, visit http://gerrit.ovirt.org/29191
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic06da1ba57868dc2c7db67a1868ad10087a1cff2
Gerrit-PatchSet: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <toni at midokura.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list