Nir Soffer has posted comments on this change. Change subject: schedule: Introduce scheduling library ......................................................................
Patch Set 14: (3 comments) http://gerrit.ovirt.org/#/c/29607/14/lib/vdsm/schedule.py File lib/vdsm/schedule.py: Line 102: """ Line 103: Schedule callable to be called after delay seconds on the scheduler Line 104: thread. Line 105: Line 106: Callable must not block or take excessive time to complete. It it does > typo: It it does Done Line 107: not finish quickly, it may delay other scheduled calls on the scheduler Line 108: thread. Line 109: Line 110: Returns a ScheduledCall that may be canceled if callable was not called Line 164: Line 165: def _cancel_calls(self): Line 166: # Help the garbage collector by breaking reference cycles Line 167: with self._cond: Line 168: for deadline, call in self._calls: > why not just empty self._calls, like this: self._calls = [] Typically an object schedule a call and keep for one of its methods, and keep a reference to the scheduler. So we have this cycle: obj -> scheduler -> calls -> scheduled_call -> method -> obj cancel() remove method from the loop, breaking the cycle. Line 169: call.cancel() Line 170: Line 171: Line 172: class ScheduledCall(object): Line 200: Line 201: Line 202: # Sentinel for marking calls as invalid. Callable so we can invalidate a call Line 203: # in a thread safe manner without locks. Line 204: def _INVALID(): > Why uppercase? This is a constant - you are not supposed to use this function. It is a function only to allow _execute to run without any racy checks. For example, we could (wrongly) implement this with None: def _execute(self): if self._callable is not None: self._callable() But it is racy. Using a function solve this race, but is function is not part of the api of this module. I think that nothing can be more clear than: self._callable = _INVALID -- To view, visit http://gerrit.ovirt.org/29607 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3764806d93bd37c3b5924080eb5ae4d29e4f4e0 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: jian wang <[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
