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.