Change in vdsm[master]: vdsm: lastclient info in jsonrpc
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jun 16 18:05:49 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: vdsm: lastclient info in jsonrpc
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.ovirt.org/#/c/28817/1//COMMIT_MSG
Commit Message:
Line 12: We can not modify API.py to provide this information in common place
Line 13: because it is not needed by mom thread which is calling this verb as
Line 14: well.
Line 15:
Line 16: The threading model of request processing has changed so we can not use
I don't see any change in the threading model in xmlrpc - there was one thread per connection, handling multiple requests, and this is still the model.
Line 17: thread local. Instead we use dictionary which is updated in
Line 18: MultiProtocolAcceptor and is read in Bridge.py and xml binding.
Line 19:
Line 20:
Line 13: because it is not needed by mom thread which is calling this verb as
Line 14: well.
Line 15:
Line 16: The threading model of request processing has changed so we can not use
Line 17: thread local. Instead we use dictionary which is updated in
Why we cannot use thread local?
Line 18: MultiProtocolAcceptor and is read in Bridge.py and xml binding.
Line 19:
Line 20:
Line 21: Change-Id: I402e15cb05f89a98dab14491d9da5985335e095e
http://gerrit.ovirt.org/#/c/28817/1/vdsm/clientIF.py
File vdsm/clientIF.py:
Line 105: port = config.getint('addresses', 'management_port')
Line 106:
Line 107: self._last_client = {}
Line 108: self._last_client['client'] = ''
Line 109: self._last_client['management_ip'] = host
This can be nicer using:
self._last_client = {'client': '', 'managment_ip': host}
But I think the keys names are problematic. I don't have any idea what is self._last_client['client'] - is this an address? adress and port? name?
'management_ip' is also strange - how is this a property of the client?
But I actually don't understand how you move the data from the thread local storage into clientIF. Any any point in time, you may have multiple connection threads, each with different client - engine, another vdsm (migration thread), or local vdsClient.
What is threadlocal.client, and why do we need it? Why having this info in clientIF is more useful?
Line 110:
Line 111: self._createAcceptor(host, port)
Line 112: self._prepareXMLRPCBinding(host, port)
Line 113: self._prepareJSONRPCBinding()
Line 120: self.mom.stop()
Line 121: raise
Line 122:
Line 123: def add(self, key, value):
Line 124: with self.dict_lock:
Do we have dict_lock? I don't see where it is initialized.
Line 125: self._last_client[key] = value
Line 126:
Line 127: def get(self, key):
Line 128: with self.dict_lock:
Line 128: with self.dict_lock:
Line 129: try:
Line 130: return self._last_client[key]
Line 131: except KeyError:
Line 132: return None
Will be more clear using:
def get(self, key):
with self.dict_lock:
return self._last_client.get(key)
Line 133:
Line 134: @property
Line 135: def ready(self):
Line 136: return (self.irs is None or self.irs.ready) and not self._recovery
--
To view, visit http://gerrit.ovirt.org/28817
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I402e15cb05f89a98dab14491d9da5985335e095e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list