Change in vdsm[master]: monitoring: Introduce the asyncevent module

nsoffer at redhat.com nsoffer at redhat.com
Wed Apr 27 18:17:17 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: monitoring: Introduce the asyncevent module
......................................................................


Patch Set 18:

(3 comments)

https://gerrit.ovirt.org/#/c/54849/18/lib/vdsm/storage/asyncevent.py
File lib/vdsm/storage/asyncevent.py:

Line 265:         This method is not implemented by Python 3 BaseEventLoop. There is no
Line 266:         way to integrate asyncore with asyncio in Python 3.
Line 267:         """
Line 268:         assert "map" not in kwargs, "map is set by the event loop"
Line 269:         return dispatcher_class(*args, map=self._channels, **kwargs)
I don't really like this solution, but the other option was to expose self._channels to callers, which seems even uglier. However, this is how asyuncore works, so maybe it is better solution, keep ugly stuff visible instead of hiding them in strange ways.
Line 270: 
Line 271:     # Testing
Line 272: 
Line 273:     def is_running(self):


Line 338:         self._callback = _CANCELLED
Line 339:         self._args = None
Line 340: 
Line 341:     @property
Line 342:     def _cancelled(self):
> this is private because it was so in python3, right?
Yes, I kept the same api, so when we move to python 3, we will not have code trying to access non existing api.

There is only one non-standard function here which is create_dispatcher, and I'm thinking about removing it.
Line 343:         return self._callback is _CANCELLED
Line 344: 
Line 345:     def _run(self):
Line 346:         try:


Line 412: 
Line 413:     The read end of the pipe is watched by the event loop, while other threads
Line 414:     may wakeup the event loop by writing a byte to the write end.
Line 415: 
Line 416:     Based on twisted.internet.posixbase._UnixWaker.
> question: did you considered eventfd? If it was discarded, could you provid
I don't like to depend on ctypes based code, and I'm not sure that the implementation is correct - it is very hard to review ctypes based code - much easier to review real c extension.

Eventfd will save us one file descriptor, and should have less overhead in the kernel, but it is not very interesting here.

If you think using event fd is worth the effort you are invited to send a patch integrating it :-)
Line 417:     """
Line 418: 
Line 419:     def __init__(self, map):
Line 420:         rfd, wfd = os.pipe()


-- 
To view, visit https://gerrit.ovirt.org/54849
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I627f06f136792967f257b6ec439622432f2021be
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list