Change in vdsm[master]: jsonrpc: events

nsoffer at redhat.com nsoffer at redhat.com
Tue May 19 20:14:10 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: jsonrpc: events
......................................................................


Patch Set 23:

(8 comments)

https://gerrit.ovirt.org/#/c/38069/23//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: Infrastructure which allows to send events.
Line 10: 
Line 11: Stomp server is no longer responsible for managing connections and all
Line 12: the connection life cycle are done in _StompConnection.
Is this related to supporting events? how?
Line 13: 
Line 14: Change-Id: Id27b5ca1773139932eb5cb16921d5abec4991c5e
Line 15: Signed-off-by: Saggi Mizrahi <smizrahi at redhat.com>


https://gerrit.ovirt.org/#/c/38069/23/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:

Line 100:         if method is None:
Line 101:             raise JsonRpcInvalidRequestError("missing method header", obj)
Line 102: 
Line 103:         reqId = obj.get("id")
Line 104:         # when sending notifications id is not provided
Why? How is this change going to effect messages which are not notifications, and are missing an event id?
Line 105: 
Line 106:         params = obj.get('params', [])
Line 107:         if not isinstance(params, (list, dict)):
Line 108:             raise JsonRpcInvalidRequestError("wrong params type", obj)


Line 184:         notification = json.dumps(
Line 185:             {
Line 186:                 'jsonrpc': '2.0',
Line 187:                 'method': self._event_id,
Line 188:                 'params': kwargs,
Why do you call params **kwargs? If an event has params, they should be called params everywhere in the application.

Also, isn't this a standard jsonrpc message? why not have a function creating a new message and use it everywhere?

For example:

    def message(method, **params):
        return {
            "jsonrpc": "2.0",
            "method": method,
            "params": params
       }
Line 189:             }
Line 190:         )
Line 191: 
Line 192:         self.log.debug("Sending event %s", notification)


Line 188:                 'params': kwargs,
Line 189:             }
Line 190:         )
Line 191: 
Line 192:         self.log.debug("Sending event %s", notification)
Is this an event or a notification? Lets use the same term everywhere.
Line 193:         self._cb(notification)
Line 194: 
Line 195: 
Line 196: class _JsonRpcClientRequestContext(object):


Line 189:             }
Line 190:         )
Line 191: 
Line 192:         self.log.debug("Sending event %s", notification)
Line 193:         self._cb(notification)
How do you know that self._cb will send the notification/event?

This is a user supplied function, so it can do anything. Why emitting a notification should be done using a user provided callback?
Line 194: 
Line 195: 
Line 196: class _JsonRpcClientRequestContext(object):
Line 197:     def __init__(self, requests, callback):


https://gerrit.ovirt.org/#/c/38069/23/lib/yajsonrpc/stomp.py
File lib/yajsonrpc/stomp.py:

Line 75: 
Line 76: class StompError(RuntimeError):
Line 77:     def __init__(self, frame):
Line 78:         self.frame = frame
Line 79:         super(RuntimeError, self).__init__(self.frame.body)
How is this change related to having events?
Line 80: 
Line 81: 
Line 82: class _HeartBeatFrame(object):
Line 83:     def encode(self):


https://gerrit.ovirt.org/#/c/38069/23/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 304
Line 305
Line 306
Line 307
Line 308
Why did you remove the close() interface?


https://gerrit.ovirt.org/#/c/38069/23/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 142:             self._send_notification,
Line 143:         )
Line 144:         notification.emit(**kwargs)
Line 145: 
Line 146:     def _send_notification(self, message, destination):
What is the unused "destination" parameter?
Line 147:         self.bindings['jsonrpc'].reactor.server.send(message)
Line 148: 
Line 149:     def contEIOVms(self, sdUUID, isDomainStateValid):
Line 150:         # This method is called everytime the onDomainStateChange


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id27b5ca1773139932eb5cb16921d5abec4991c5e
Gerrit-PatchSet: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets 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: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list