Nir Soffer has posted comments on this change.

Change subject: schedule: Introduce scheduling libary
......................................................................


Patch Set 4:

(4 comments)

Francesco, see my response in the inline comment.

http://gerrit.ovirt.org/#/c/29607/4/lib/vdsm/schedule.py
File lib/vdsm/schedule.py:

Line 61:     """
Line 62:     Schedule calls for future execution in a background thread.
Line 63: 
Line 64:     This class is thread safe; multiple threads can schedule calls or 
cancel
Line 65:     the scheudler.
> typo: scheduler
Will fix
Line 66:     """
Line 67: 
Line 68:     DEFAULT_DELAY = 30.0  # Used if no call are scheduled
Line 69: 


Line 77:         t = threading.Thread(target=self._run, name=name)
Line 78:         t.daemon = True
Line 79:         t.start()
Line 80: 
Line 81:     def schedule(self, delay, callable):
> 'callable' shadows a builtin, that is usually a thing to avoid
Was callle before, changed to callable as Saggi suggested. This hides the 
builtin but this code does not use, so I'm fine with that.

Unless you come up with better name that is clear as "callable".
Line 82:         """
Line 83:         Schedule callable to be called after delay seconds on the 
scheduler
Line 84:         thread.
Line 85: 


Line 82:         """
Line 83:         Schedule callable to be called after delay seconds on the 
scheduler
Line 84:         thread.
Line 85: 
Line 86:         Callable must not block or take excessive time to complete. It 
it does
> Still, in sampling we can have calls that block, because some calls may nee
You cannot call this if your call blocks. When used for sampling, you will 
schedule the call to put the task in the threadpool, which cannot block:

Here is an example Sampler to illustrate this usage:

    class Sampler:

        # Task interface

        def execute(self):
            self.status = RUNNING
            self._collect_stats()
            self.scheduler.schedule(15, self.dispatch)
            self.status = WAITING

        @property
        def status(self):
            return self.status

        # Private

        def _dispatch(self):
            self.threadpool.dispatch(self)

So there is no issue here, because  handling calls that block is the 
responsibility of the caller.
Line 87:         not finish quickly, it may delay other scheduled calls on the 
scheduler
Line 88:         thread.
Line 89: 
Line 90:         Returns a ScheduledCall that may be canceled if callable was 
not called


Line 91:         yet.
Line 92:         """
Line 93:         deadline = time.time() + delay
Line 94:         call = _Call(deadline, callable)
Line 95:         self._log.debug("Schedulng %s", call)
> typo: scheduling
Will fix
Line 96:         with self._cond:
Line 97:             if self._running:
Line 98:                 heapq.heappush(self._calls, call)
Line 99:                 self._cond.notify()


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <[email protected]>
Gerrit-Reviewer: Adam Litke <[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

Reply via email to