Nir Soffer has posted comments on this change. Change subject: executor: introduce the executor library ......................................................................
Patch Set 12: (5 comments) I think this should be rebased on the schedule patch, under the sampling patch, so we can use this executor in the sampling patch. http://gerrit.ovirt.org/#/c/29191/12/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 52: self._tasks = _Queue(max_tasks) Line 53: self._workers = set() Line 54: self._cond = threading.Condition(threading.Lock()) Line 55: self._running = False Line 56: self._scheduler = scheduler Can you initialize the object instance variables in the same order of the arguments, and do this before any other thing? While this does not make any difference in the behavior, It makes it easier to understand the code. Line 57: self._worker_id = 0 Line 58: Line 59: def start(self): Line 60: self._log.debug('Starting executor') Line 101: worker.busy = False Line 102: self._log.debug('Starting worker %s' % worker.name) Line 103: worker.start() Line 104: self._workers.add(worker) Line 105: self._workers.add(worker) I think that one of these is enough :-) Line 106: Line 107: def _task_discarded(self): Line 108: with self._cond: Line 109: if self._running: Line 186: on_task_discarded = self._on_task_discarded Line 187: on_task_discarded() Line 188: Line 189: Line 190: class _Queue(object): Lets call it _TaskQueue, or rename the _max_tasks, and _tasks to more generic names. Since this is not really a generic utility, making it more specific seems more correct. Line 191: """Queue.Queue doesn't support the clear() operation. Line 192: All the locking must be provided by the caller.""" Line 193: Line 194: def __init__(self, max_tasks): Line 194: def __init__(self, max_tasks): Line 195: self._max_tasks = max_tasks Line 196: self._tasks = collections.deque() Line 197: Line 198: @property This does not makes sense as a property. Line 199: def empty(self): Line 200: return len(self._tasks) == 0 Line 201: Line 202: def put(self, task): Line 207: def get(self): Line 208: return self._tasks.pop() Line 209: Line 210: def clear(self): Line 211: self._tasks.clear() Nice! I ignored this direction because of the locking issues, but without locking this makes the code much nicer. Line 212: Line 213: Line 214: class _ExecutorStopped(Exception): Line 215: pass -- 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: 12 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
