Francesco Romani has posted comments on this change. Change subject: executor: introduce the executor library ......................................................................
Patch Set 32: (7 comments) Yes, I could have clean it up a bit more. Thanks for the reminder! http://gerrit.ovirt.org/#/c/29191/32/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 81: workers = tuple(self._workers) if wait else () Line 82: for worker in workers: Line 83: try: Line 84: worker.join() Line 85: except _WorkerBusy: > Nobody is raising this now - should be deleted. Done Line 86: self._log.debug('Skipping busy worker %s', worker.name) Line 87: Line 88: def dispatch(self, callable, timeout=None): Line 89: """ Line 138: _STOP = object() Line 139: Line 140: Line 141: class _WorkerBusy(Exception): Line 142: """ Raised when trying to join a busy worker """ > Unused, delete Done Line 143: Line 144: Line 145: class _WorkerDiscarded(Exception): Line 146: """ Raised if worker was discarded during execution of a task """ Line 192: callable() Line 193: except Exception: Line 194: self._log.exception("Unhandled exception in %s", callable) Line 195: finally: Line 196: self._busy = False > We can simplify by removing the _busy instance variable, and cancel discard Done Line 197: if self._discarded: Line 198: raise _WorkerDiscarded() Line 199: if discard is not None: Line 200: discard.cancel() Line 207: def _discard(self): Line 208: if self._discarded: Line 209: raise AssertionError("Attempt to discard worker twice") Line 210: if not self._busy: Line 211: return > Do we need this check? Yes, seems now useless. It seems also there is a race between discard timer and the timer disarm in lines 197-200. If so, this was probably added to limit the damage in case of that race. Maybe worth a comment. Line 212: self._discarded = True Line 213: self._log.debug("Worker %s discarded", self.name) Line 214: self._executor._worker_discarded(self) Line 215: Line 214: self._executor._worker_discarded(self) Line 215: Line 216: Line 217: class _TaskQueue(object): Line 218: """ Queue.Queue doesn't support the clear() operation. """ > This is not a good docstring. Lets describe what this does and use this lin Done Line 219: Line 220: def __init__(self, max_tasks): Line 221: self._max_tasks = max_tasks Line 222: self._tasks = collections.deque() Line 225: # https://docs.python.org/2.6/library/queue.html#Queue.Full Line 226: self._cond = threading.Condition(threading.Lock()) Line 227: Line 228: def put(self, task): Line 229: # there is a race here but we don't really mind. Worst case scenario, > there -> There Done Line 230: # we will have a bit more task then the configured maximum, but we Line 231: # just want to avoid to have indefinite amount of tasks. Line 232: if len(self._tasks) == self._max_tasks: Line 233: raise TooManyTasks() Line 227: Line 228: def put(self, task): Line 229: # there is a race here but we don't really mind. Worst case scenario, Line 230: # we will have a bit more task then the configured maximum, but we Line 231: # just want to avoid to have indefinite amount of tasks. > Please add also that we don't want to block. Otherwise we could use the bui Done Line 232: if len(self._tasks) == self._max_tasks: Line 233: raise TooManyTasks() Line 234: self._tasks.append(task) Line 235: with self._cond: -- 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: 32 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
