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