Change in vdsm[master]: json-rpc: Protocol detection

nsoffer at redhat.com nsoffer at redhat.com
Thu Jun 5 12:53:36 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 32:

(1 comment)

http://gerrit.ovirt.org/#/c/26300/32/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:

Line 488:             res = True if res is None else res
Line 489: 
Line 490:             ctx.requestDone(JsonRpcResponse(res, None, req.id))
Line 491: 
Line 492:     @traceback(on=log.name)
> Nir suggested to use it.
I was suggesting this on the protocol detector, not here, because I did not review this file.

This decorator makes code simpler because you don't need to add try blocks around the body of threads target function, and it will not break when your error handling code is broken.

This does not replace proper error handling in the thread, but a last resort error handler, ensuring that when the thread dies in the field (because error handling was broken), it will be logged.

So instead of:

    def mainloop(self):
        try:
            do stuff
            and other stuff
            even more
        except Exception:
            logging.exception("thread foo died")

You can do:

    @traceback(on=log.name, msg="thread foo died")
    def mainloop(self):
        do stuff
        and other stuff
        even more

Looking in function body, there is no error handling in the while loop, so any unexpected failure in one of statements will cause the thread to abort silently, which makes it impossible to support this code in the field.
Line 493:     def serve_requests(self):
Line 494:         while True:
Line 495:             self.log.debug("Waiting for request")
Line 496:             obj = self._workQueue.get()


-- 
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan 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