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(a)redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes