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