Francesco Romani has posted comments on this change. Change subject: scheduler: use single instance ......................................................................
Patch Set 6: (4 comments) despite -1 (mostly for visibility), this is actually almost OK. Few comments inside. https://gerrit.ovirt.org/#/c/43825/6/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 53: def start(cif, scheduler): Line 54: global _operations Line 55: global _executor Line 56: Line 57: _executor = executor.Executor(name="periodic.Executor", Well, this is not needed. Let's just keep this name "periodic". Line 58: workers_count=_WORKERS, Line 59: max_tasks=_TASKS, Line 60: scheduler=scheduler) Line 61: _executor.start() Line 110: def stop(): Line 111: for op in _operations: Line 112: op.stop() Line 113: Line 114: if _executor is not None: this check is useful, but unrelated. Please either describe why is needed in this patch or move it into another one. Line 115: _executor.stop(wait=False) Line 116: Line 117: Line 118: class Operation(object): Line 125: """ Line 126: Line 127: _log = logging.getLogger("virt.periodic.Operation") Line 128: Line 129: def __init__(self, func, period, scheduler, executor, timeout=0): ok for scheduler, not ok for executor (I don't see the reason to change, see also commet on line 145). let's have def __init__(self, func, period, scheduler, timeout=0, executor=None): # ... Line 130: """ Line 131: parameters: Line 132: Line 133: func: callable, without arguments (task interface). Line 141: self._func = func Line 142: self._period = period Line 143: self._timeout = _timeout_from(period) if timeout == 0 else timeout Line 144: self._scheduler = scheduler Line 145: self._executor = executor ok for line 144 about scheduler, but I'd still like to have fallback to implicit module executor: there is no reason in sight to bind an operation to a different scheduler. So please drop the change to line 145. Line 146: self._lock = threading.Lock() Line 147: self._running = False Line 148: self._call = None Line 149: -- To view, visit https://gerrit.ovirt.org/43825 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If83eded458f8304d802fcd75839e7a916146918b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
