Nir Soffer has posted comments on this change.

Change subject: json-rpc: Protocol detection
......................................................................


Patch Set 29: Code-Review-1

(5 comments)

This patch breaks code in vdsm/virt/migration.py

Adding virt folks to review this change.

http://gerrit.ovirt.org/#/c/26300/29/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 82:         self._recovery = True
Line 83:         self.channelListener = Listener(self.log)
Line 84:         self._generationID = str(uuid.uuid4())
Line 85:         self.mom = None
Line 86:         self._bindings = []
bindings was public in the previous code, and is used from 
vdsm/virt/migration.py, which expect it to be a dictionary. This usage is 
questionable and should probably be removed, but lets keep this change minimal 
and clean up migration.py later.

Please rename it to self.bindings and use a dictionary instead of a list.

When you verify again, please use 2 hosts an perform migration.
Line 87:         if _glusterEnabled:
Line 88:             self.gluster = gapi.GlusterApi(self, log)
Line 89:         else:
Line 90:             self.gluster = None


Line 181:             else:
Line 182:                 default_bridge = config.get("vars", "default_bridge")
Line 183:                 xml_binding = BindingXMLRPC(self, self.log, host,
Line 184:                                             default_bridge)
Line 185:                 self._bindings.append(xml_binding)
Use same line from the old code:

    self.binding['xmlrpc'] = xml_binding
Line 186:                 xml_detector = XmlDetector(xml_binding)
Line 187:                 self._acceptor.add_detector(xml_detector)
Line 188: 
Line 189:     def _prepareJSONRPCBinding(self):


Line 197:                               'Please make sure it is installed.')
Line 198:             else:
Line 199:                 bridge = Bridge.DynamicBridge()
Line 200:                 json_binding = BindingJsonRpc(bridge)
Line 201:                 self._bindings.append(json_binding)
Use same line from the old code:

    self.binding['json'] = json_binding
Line 202:                 stomp_detector = StompDetector(json_binding)
Line 203:                 self._acceptor.add_detector(stomp_detector)
Line 204: 
Line 205:     def _prepareMOM(self):


Line 232:                 self.log.debug('cannot run prepareForShutdown twice')
Line 233:                 return errCode['unavail']
Line 234: 
Line 235:             self._acceptor.stop()
Line 236:             for binding in self._bindings:
Use same line from old code:
    
    for binding in self.bindings.values():
Line 237:                 binding.stop()
Line 238: 
Line 239:             self._enabled = False
Line 240:             self.channelListener.stop()


Line 248:         finally:
Line 249:             self._shutdownSemaphore.release()
Line 250: 
Line 251:     def start(self):
Line 252:         for binding in self._bindings:
Use same line from old code:

    for binding in self.bindings.values():
Line 253:             binding.start()
Line 254:         self.thread = 
threading.Thread(target=self._acceptor.serve_forever,
Line 255:                                        name='Detector thread')
Line 256:         self.thread.setDaemon(True)


-- 
To view, visit http://gerrit.ovirt.org/26300
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75
Gerrit-PatchSet: 29
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to