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