Saggi Mizrahi has posted comments on this change.
Change subject: schedule: Introduce scheduling libary ......................................................................
Patch Set 3: Code-Review-1
(13 comments)
I also prefer using_underscores but mixedCase is how things are in VDSM.
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") Just 'Scheduler' or at the least 'vdsm.Scheduler' 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()) Just use an empty ctor. Why overcomplicate? 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) Please name the thread. 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): callee should be callable or target to keep with the standard library's convention. 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: I would raise an error since getting here is likely a bug. 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) I don't like utils.traceback.
It's silly and should be removed from the codebase. It removes proper context from the log message and moves the intent to log to a weird place IMHO (the function deceleration) Please don't use it in anything that is going to become infra code. 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): _mixedCase 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 don't like implied False. Check what you mean to check. len(self._timeouts) > 0 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 Just wait for eternity Line 141: Line 142: def _pop_expired_timeouts(self): Line 143: now = time.time() Line 144: expired = []
Line 138: if self._timeouts: Line 139: return self._timeouts[0].deadline - time.time() Line 140: return self.DEFAULT_DELAY Line 141: Line 142: def _pop_expired_timeouts(self): _mixedCase Line 143: now = time.time() Line 144: expired = [] Line 145: while self._timeouts: Line 146: timeout = self._timeouts[0]
Line 155: with self._cond: Line 156: for timeout in self._timeouts: Line 157: timeout.cancel() Line 158: Line 159: Seems like _Timeout and ScheduledCall are actually both ScheduledCall but one interface is for internal use and one is for external.
That is the problem with not having interfaces in python.
In any case having a class name _Timeout is a bit odd.
I'd call the internal one ScheduledCall and the public one ScheduledCallToken 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.
Line 196: self.deadline = deadline Line 197: self.callee = callee Line 198: Line 199: def fire(self): Line 200: if self.callee is _INVALID: Just don't check, making it a function was the correct thing to do. Line 201: return Line 202: try: Line 203: self.callee() Line 204: except Exception:
Line 201: return Line 202: try: Line 203: self.callee() Line 204: except Exception: Line 205: self._log.exception("Unhandled exception in scheduled call") exc_info Line 206: finally: Line 207: self.callee = _INVALID Line 208: Line 209: def cancel(self):