Saggi Mizrahi has posted comments on this change. Change subject: schedule: Introduce scheduling libary ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/29607/3/lib/vdsm/schedule.py File lib/vdsm/schedule.py: Line 66: """ Line 67: Line 68: DEFAULT_DELAY = 30.0 # Used if no timeout are scheduled Line 69: Line 70: _log = logging.getLogger("vds.Scheduler") > vds will inherit the vds logger configuration. We may want to change the be I don't see why this needs to inherit the vds log configuration. Line 71: Line 72: def __init__(self): Line 73: self._log.debug("Starting scheduler") Line 74: self._cond = threading.Condition(threading.Lock()) Line 70: _log = logging.getLogger("vds.Scheduler") Line 71: Line 72: def __init__(self): Line 73: self._log.debug("Starting scheduler") Line 74: self._cond = threading.Condition(threading.Lock()) > The default lock type is RLock, which is not needed in this case. That's odd since using RLock and a Condition is highly frowned upon, but then again python was never known for being smart about it's sync primitives. Line 75: self._running = True Line 76: self._timeouts = [] Line 77: t = threading.Thread(target=self._run) Line 78: t.daemon = True Line 73: self._log.debug("Starting scheduler") Line 74: self._cond = threading.Condition(threading.Lock()) Line 75: self._running = True Line 76: self._timeouts = [] Line 77: t = threading.Thread(target=self._run) > Good idea. I'm cool with that. Line 78: t.daemon = True Line 79: t.start() Line 80: Line 81: def schedule(self, delay, callee): Line 77: t = threading.Thread(target=self._run) Line 78: t.daemon = True Line 79: t.start() Line 80: Line 81: def schedule(self, delay, callee): > I'll change to callable, hopefully Dan will not complain that it hides the Yea, it is a problem. But callee isn't really a word. I guess using target is the way to go. (similar to thread) Line 82: """ Line 83: Schedule callee to be called after delay seconds on the scheduler Line 84: thread. Line 85: Line 96: with self._cond: Line 97: if self._running: Line 98: heapq.heappush(self._timeouts, timeout) Line 99: self._cond.notify() Line 100: else: > This can be normal condition, you have one thread that does sampling, and a I'm ok with logging Line 101: timeout.cancel() Line 102: return ScheduledCall(timeout) Line 103: Line 104: def cancel(self): Line 110: with self._cond: Line 111: self._running = False Line 112: self._cond.notify() Line 113: Line 114: @utils.traceback(on=_log.name) > Lets talk about the technical reasons. The problem you are trying to fix is not an issue we should care about. Error with the log formatting are not issues that I need to expect. If there is a bug in in the logging code it's something that needs to be fixed. If we want to make sure exceptions in threads go to a logger instead of standard error that is a different problem all together. log lines needs to be explicitly put near the issue. logging is never a property of the function, I know that there are places in the code were decorators are used for logging but they will need to be eventually removed. Logging needs to be placed in a predictable place. Line 115: def _run(self): Line 116: try: Line 117: self._log.debug("started") Line 118: self._loop() Line 133: expired = self._pop_expired_timeouts() Line 134: for timeout in expired: Line 135: timeout.fire() Line 136: Line 137: def _time_until_deadline(self): > pep8 I prefer that style as well but I much rather have uniformity than adhere to a specific style. This kind of mixing might be tolerable in a strict language but with duck typing you are just making it harder for people to remember the function names. Line 138: if self._timeouts: Line 139: return self._timeouts[0].deadline - time.time() Line 140: return self.DEFAULT_DELAY Line 141: Line 134: for timeout in expired: Line 135: timeout.fire() Line 136: Line 137: def _time_until_deadline(self): Line 138: if self._timeouts: > I think this is little bit too noisy; this is the clearest way to say what That is how we do things. We had issues with that style of coding. We are not using it. Line 139: return self._timeouts[0].deadline - time.time() Line 140: return self.DEFAULT_DELAY Line 141: Line 142: def _pop_expired_timeouts(self): Line 136: Line 137: def _time_until_deadline(self): Line 138: if self._timeouts: Line 139: return self._timeouts[0].deadline - time.time() Line 140: return self.DEFAULT_DELAY > This will complicate the calling code for no reason. I can change the defau Why would it complicate? Line 141: Line 142: def _pop_expired_timeouts(self): Line 143: now = time.time() Line 144: expired = [] Line 155: with self._cond: Line 156: for timeout in self._timeouts: Line 157: timeout.cancel() Line 158: Line 159: > ScheduledCallToken is too long and confusing - this is not a token. Token i Token is the thing you hold. And I'm OK with that scheme as long as you document that one is the public interface since it's not immediately clear. Line 160: class ScheduledCall(object): Line 161: """ Line 162: Returned when a callable is scheduled to be called after delay. The caller Line 163: may cancel the call if it was not called yet. -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[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: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
