Nir Soffer has posted comments on this change.
Change subject: schedule: Introduce scheduling libary ......................................................................
Patch Set 3:
(13 comments)
Thanks for the quick review!
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'
vds will inherit the vds logger configuration. We may want to change the behavior of infrastructure related logging.
Personally I like shorter logger names, but having common configuration is useful. 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?
The default lock type is RLock, which is not needed in this case. 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.
Good idea.
I think we should do:
def __init__(self, name="Scheduler"): ... t = threading.Thread(target=self._run, name=name)
So if we start more then one scheduler, we can name them easily. 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 con
I'll change to callable, hopefully Dan will not complain that it hides the builtin function :-) 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.
This can be normal condition, you have one thread that does sampling, and another thread stop the scheduler, for example, during shutdown. In this case a traceback is unwanted, and we don't want to catch this error on all calls to the scheduler, to prevent bogus warnings on shutdown.
Maybe log a warning here? - this will help us fix bad code that use the scheduler when it should not. 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.
Lets talk about the technical reasons.
This utility is critical to allow debugging when thread dies. This works even if the error handling code in the thread is broken, for example:
try: do stuff.. except Exception: self.log.debug("%s", nosuchname)
In this case we will get the name error in the log. Otherwise the thread will just die silently.
This utility was created per Federico request, when we tried to debug a domain monitor thread that was disappeared silently. This bug moved to infra and finally close without resolution. If we had this decorator there, we could solve the original issue.
If you want better log message, we can use the msg argument:
@utils.traceback(on=log.name, msg="whatever")
Or improve the decorator :-) 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
pep8 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.
I think this is little bit too noisy; this is the clearest way to say what I want to say in Python. 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
This will complicate the calling code for no reason. I can change the default timeout to bigger value (e.g. 300) if you think that it make any difference. 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
pep8 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
ScheduledCallToken is too long and confusing - this is not a token. Token is not something that you cancel.
The public class is the what you return from schedule, so the name makes sense. The private one is related to the implementation, which use a list of timeouts.
Your view that both are actually the same class is correct, so we can use the same name:
- ScheculedCall - public - _ScheduledCall - private
And change _timeouts to _calls 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.
ok 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
This logs an exception - why do you want to add exc_info? Line 206: finally: Line 207: self.callee = _INVALID Line 208: Line 209: def cancel(self):