Nir Soffer has posted comments on this change.
Change subject: schedule: Introduce scheduling libary ......................................................................
Patch Set 3:
(8 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")
I don't see why this needs to inherit the vds log configuration.
Done Line 71: Line 72: def __init__(self): Line 73: self._log.debug("Starting scheduler") Line 74: self._cond = threading.Condition(threading.Lock())
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)
I'm cool with that.
Done 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):
Yea, it is a problem. But callee isn't really a word. I guess using target
Changed to callable, target is a poor name for something that you can call. 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'm ok with logging
Done 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)
The problem you are trying to fix is not an issue we should care about. Err
I think that the traceback decorator is the best option for ensuring that thread main function always log fatal failures.
Adding explicit logging will required a second try except for every thread main function:
try: try: loop... finally: cleanup... except Exception: log exception
This bolierplate does not help anyone, and does not give more context - both will give the same useful traceback.
Lets see what others think about it. 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):
I prefer that style as well but I much rather have uniformity than adhere t
But this is not a public api, nobody has to type this name. 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:
That is how we do things. We had issues with that style of coding. We are n
I will use the more verbose way to keep you happy :-) 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
Why would it complicate?
The calling code get a value and wait only if the value is positive. This way we save a syscall when we have an expired timeout to fire, and the code is the most simple and clear that we can have.
The 30 seconds timeout does not have any effect on the system, it is equivalent to waiting for eternity. Line 141: Line 142: def _pop_expired_timeouts(self): Line 143: now = time.time() Line 144: expired = []