Change in vdsm[master]: jsonrpc: Create the java bindings and fix bugs

ybronhei at redhat.com ybronhei at redhat.com
Wed Oct 2 11:56:50 UTC 2013


Yaniv Bronhaim has posted comments on this change.

Change subject: jsonrpc: Create the java bindings and fix bugs
......................................................................


Patch Set 5:

(18 comments)

....................................................
File lib/yajsonrpc/__init__.py
Line 282:             if cb is None or cb == eventcb:
Line 283:                 try:
Line 284:                     self._eventcbs.remove(r)
Line 285:                 except ValueError:
Line 286:                     pass
report? print r?
Line 287: 
Line 288:     def emit(self, client, event, params):
Line 289:         for r in self._eventcbs[:]:
Line 290:             cb = r()


Line 315:             try:
Line 316:                 mobj = json.loads(message)
Line 317:                 isResponse = self._isResponse(mobj)
Line 318:             except:
Line 319:                 self.log.warning("Problem parsing message from client")
better to print the message for debugging, no?
Line 320:                 transport.close()
Line 321:                 del self._clients[transport]
Line 322:                 continue
Line 323: 


Line 333:                     if v is None:
Line 334:                         v = res
Line 335: 
Line 336:                     if v != res:
Line 337:                         raise TypeError("batch is mixed")
print also v and res for more details
Line 338: 
Line 339:                 return v
Line 340:             else:
Line 341:                 return ("result" in obj or "error" in obj)


Line 340:             else:
Line 341:                 return ("result" in obj or "error" in obj)
Line 342: 
Line 343:     def close(self):
Line 344:         self._inbox.put(None)
why not using a flag and stop serving immediately when close is called? in this case you continue to serve until _inbox gets to None object, instead of having boolean for the while loop that 'close' changes to false.
Line 345: 
Line 346: 
Line 347: class JsonRpcCall(object):
Line 348:     def __init__(self):


....................................................
File lib/yajsonrpc/asyncoreReactor.py
Line 100:         if sslctx is None:
Line 101:             try:
Line 102:                 sslctx = sock.get_context()
Line 103:             except AttributeError:
Line 104:                 pass
please report.
Line 105:         else:
Line 106:             sslctx = _SSLContextFactory(sslctx)
Line 107: 
Line 108:         if sslctx is None:


Line 177:         self.log.debug("Accepting connection from client "
Line 178:                        "at tcp://%s:%s", addr[0], addr[1])
Line 179: 
Line 180:         clientWrapper = AsyncoreClient(sock, self._reactor, addr)
Line 181:         self._acceptHandler(self, clientWrapper)
no need for the Wrapper suffix
Line 182: 
Line 183:     def writable(self, dispatcher):
Line 184:         return False
Line 185: 


....................................................
File lib/yajsonrpc/betterAsyncore.py
Line 105: class AsyncChat(object):
Line 106:     # these are overridable defaults
Line 107: 
Line 108:     ac_in_buffer_size = 4096
Line 109:     ac_out_buffer_size = 4096
can be constants?
Line 110: 
Line 111:     def __init__(self, impl):
Line 112:         self._fifoLock = Lock()
Line 113:         self._impl = impl


Line 147: 
Line 148:     # grab some more data from the socket,
Line 149:     # throw it to the collector method,
Line 150:     # check for the terminator,
Line 151:     # if found, transition to the next state.
is this comment refer to something around?
Line 152: 
Line 153:     def handle_read(self, dispatcher):
Line 154: 
Line 155:         try:


Line 154: 
Line 155:         try:
Line 156:             data = dispatcher.recv(self.ac_in_buffer_size)
Line 157:         except socket.error:
Line 158:             dispatcher.handle_error()
verify that it prints log here, otherwise add such print
Line 159:             return
Line 160: 
Line 161:         self.ac_in_buffer = self.ac_in_buffer + data
Line 162: 


Line 208:                 else:
Line 209:                     # check for a prefix of the terminator
Line 210:                     index = self._find_prefix_at_end(self.ac_in_buffer,
Line 211:                                                      terminator)
Line 212:                     if index:
index > 0 please. index=-1 gets in also
Line 213:                         if index != lb:
Line 214:                             # we found a prefix, collect up to the prefix
Line 215:                             self.collect_incoming_data(
Line 216:                                 self.ac_in_buffer[:-index])


Line 222:                         self.ac_in_buffer = ''
Line 223: 
Line 224:     def _find_prefix_at_end(haystack, needle):
Line 225:         l = len(needle) - 1
Line 226:         while l and not haystack.endswith(needle[:l]):
while l > 0 , if it gets below zero by mistake somehow you can end up with infinite loop
Line 227:             l -= 1
Line 228:         return l
Line 229: 
Line 230:     def handle_write(self, dispatcher):


Line 318: 
Line 319:         try:
Line 320:             impl.init(self)
Line 321:         except AttributeError:
Line 322:             pass
better to report before ignoring, no?
Line 323: 
Line 324:     def __invoke(self, name, *args, **kwargs):
Line 325:         if hasattr(self.__impl, name):
Line 326:             return getattr(self.__impl, name)(self, *args, **kwargs)


Line 373: 
Line 374:     def connect(self, addr):
Line 375:         self.connected = False
Line 376:         self.connecting = True
Line 377:         socket = self.socket
why not using self.socket directly ?
Line 378:         socket.setblocking(1)
Line 379:         socket.connect(addr)
Line 380:         socket.setblocking(0)
Line 381:         self.addr = addr


....................................................
File lib/yajsonrpc/protonReactor.py
Line 62:         self._reactor = reactor
Line 63:         self._connected = False
Line 64: 
Line 65:     def setTimeout(self, timeout):
Line 66:         # TODO
maybe a log print that it is being used without appropriate implementation ?
Line 67:         pass
Line 68: 
Line 69:     def closed(self):
Line 70:         return (self.connector is None or


Line 85: 
Line 86:         self.connector = proton.pn_connector(self._reactor._driver,
Line 87:                                              host, str(port), None)
Line 88:         if self.connector is None:
Line 89:             # TODO: proper exception
Why delaying it? add proper exception here.
Line 90:             raise Exception("Could not create connector")
Line 91: 
Line 92:         self.connection = proton.pn_connection()
Line 93:         proton.pn_connector_set_connection(self.connector, self.connection)


....................................................
File tests/jsonRpcTests.py
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: import threading
Line 21: import socket
Line 22: import logging
Any logging config somewhere?
Line 23: from Queue import Queue
Line 24: from contextlib import contextmanager
Line 25: from testValidation import brokentest
Line 26: 


....................................................
File tests/jsonRpcUtils.py
Line 57:            ("127.0.0.1", port))
Line 58: 
Line 59: 
Line 60: REACTOR_CONSTRUCTORS = {"tcp": _tcpServerConstructor,
Line 61:                         "amqp": _protonServerConstructor}
why changing the proton key to amqp? and if so, change also _protonServerConstructor to _amqp...
Line 62: REACTOR_TYPE_PERMUTATIONS = [[r] for r in REACTOR_CONSTRUCTORS.iterkeys()]
Line 63: SSL_OPTIONS = (True, False)
Line 64: CONNECTION_PERMUTATIONS = tuple(product(REACTOR_CONSTRUCTORS.iterkeys(),
Line 65:                                         SSL_OPTIONS))


Line 59: 
Line 60: REACTOR_CONSTRUCTORS = {"tcp": _tcpServerConstructor,
Line 61:                         "amqp": _protonServerConstructor}
Line 62: REACTOR_TYPE_PERMUTATIONS = [[r] for r in REACTOR_CONSTRUCTORS.iterkeys()]
Line 63: SSL_OPTIONS = (True, False)
can you add short comment about the usage of this tupple ? what True and False mean?. it isn't being used in this file, so its hard to know by reading it here
Line 64: CONNECTION_PERMUTATIONS = tuple(product(REACTOR_CONSTRUCTORS.iterkeys(),
Line 65:                                         SSL_OPTIONS))
Line 66: 
Line 67: CERT_DIR = os.path.abspath(os.path.dirname(__file__))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If828355b7efe28fe6a2e784069425fefd2f3f25c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list