Francesco Romani has posted comments on this change. Change subject: scheduler: use single instance ......................................................................
Patch Set 7: Code-Review-1 (4 comments) thanks for the update. Unfortunately we need a few more fixes to make this patch minimal. We have a few nits here, the most important part is that I don't want to hide a possible bug by mistake. https://gerrit.ovirt.org/#/c/43825/7/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 62: Line 63: def per_vm_operation(func, period): Line 64: disp = VmDispatcher( Line 65: cif.getVMs, _executor, func, _timeout_from(period)) Line 66: return Operation(disp, period, scheduler, _executor) if we use the default executor, let's avoid to pass it. We'll make things more explicit, if needed, in a separate patch. Thus: return Operation(disp, period, scheduler) Line 67: Line 68: _operations = [ Line 69: # needs dispatching becuse updating the volume stats needs the Line 70: # access the storage, thus can block. Line 91: cif.getVMs, Line 92: sampling.stats_cache), Line 93: config.getint('vars', 'vm_sample_interval'), Line 94: scheduler, Line 95: _executor), same as per line 66. It's fine to pass scheduler, but let's keep this change minimal, so Operation( sampling.VMBulkSampler( libvirtconnection.get(cif), cif.getVMs, sampling.stats_cache), config.getint('vars', 'vm_sample_interval'), scheduler), Line 96: Line 97: # we do this only until we get high water mark notifications Line 98: # from qemu. Access storage and/or qemu monitor, so can block, Line 99: # thus we need dispatching. Line 111: for op in _operations: Line 112: op.stop() Line 113: Line 114: if _executor is not None: Line 115: _executor.stop(wait=False) your intention here is good, but if this is None, it means that someone called stop() without having called start() before, thus we'll hide a bug. I'd rather explode to expose the bug. Please revert. Line 116: Line 117: Line 118: class Operation(object): Line 119: """ Line 125: """ Line 126: Line 127: _log = logging.getLogger("virt.periodic.Operation") Line 128: Line 129: def __init__(self, func, period, scheduler, executor=None, timeout=0): please switch the order of "executor" and "timeout": def __init__(self, func, period, scheduler, timeout=0, executor=None): we should order the default argument to have the less likely to change as last one. Line 130: """ Line 131: parameters: Line 132: Line 133: func: callable, without arguments (task interface). -- 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: 7 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
