Piotr Kliczewski has posted comments on this change. Change subject: [WIP] EVENTS ......................................................................
Patch Set 1: Code-Review-1 (14 comments) http://gerrit.ovirt.org/#/c/38069/1/lib/stompClient.py File lib/stompClient.py: Line 74: Line 75: Line 76: BROKER_ADDRESS = ("127.0.0.1", 54321) Line 77: Line 78: # server = dummy_server(BROKER_ADDRESS) Do we need this comment? Line 79: Line 80: time.sleep(2) Line 81: Line 82: client = connect(BROKER_ADDRESS) http://gerrit.ovirt.org/#/c/38069/1/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 324: ('management_ip', '0.0.0.0', 'Set to "::" to listen on IPv6.'), Line 325: Line 326: ('guests_gateway_ip', '', None), Line 327: Line 328: ('broker_address', '127.0.0.1', Do we really bind to loopback? Line 329: 'Address where the broker is listening at. Use an empty string ' Line 330: 'for none'), Line 331: Line 332: ('broker_port', '5445', http://gerrit.ovirt.org/#/c/38069/1/lib/yajsonrpc/stompReactor.py File lib/yajsonrpc/stompReactor.py: Line 28: _STATE_LEN = "Waiting for message length" Line 29: _STATE_MSG = "Waiting for message" Line 30: Line 31: Line 32: _DEFAULT_RESPONSE_DESTINATION = "jms.topic.vdsm_legacy_responses" How that will work with current 3.5? Line 33: _DEFAULT_REQUEST_DESTINATION = "jms.topic.vdsm_legacy_requests" Line 34: Line 35: Line 36: def parseHeartBeatHeader(v): Line 348: def __init__(self, client, request_frame): Line 349: self._client = client Line 350: self._reply_to = request_frame.headers.get('reply-to', None) Line 351: Line 352: def get_local_address(self, *args, **kwargs): Do we need it? Line 353: return "" Line 354: Line 355: def send(self, data): Line 356: if self._reply_to is None: http://gerrit.ovirt.org/#/c/38069/1/tests/miscTests.py File tests/miscTests.py: Line 990: gc.collect() Line 991: openFds = openFdNum() Line 992: self.testStdOut() Line 993: gc.collect() Line 994: print gc.garbage what is the value of printing it? Line 995: self.assertEquals(len(gc.garbage), 0) Line 996: self.assertEquals(openFdNum(), openFds) Line 997: Line 998: http://gerrit.ovirt.org/#/c/38069/1/tests/protocoldetectorTests.py File tests/protocoldetectorTests.py: Line 66: while "\n" not in request: Line 67: request += dispatcher.recv(1024) Line 68: Line 69: response = self.response(request) Line 70: client_socket.setblocking(1) Why do we want to block? Line 71: client_socket.sendall(response) Line 72: finally: Line 73: client_socket.shutdown(socket.SHUT_RDWR) Line 74: client_socket.close() Line 214: '127.0.0.1', Line 215: 0, Line 216: sslctx=self.SSLCTX if use_ssl else None Line 217: ) Line 218: self.acceptor._handshake_timeout = 1 why do we need this timeout? Line 219: self.acceptor.add_detector(Echo()) Line 220: self.acceptor.add_detector(Uppercase()) Line 221: self.acceptor_address = self.acceptor._acceptor.socket.getsockname() Line 222: t = threading.Thread(target=self.reactor.process_requests) http://gerrit.ovirt.org/#/c/38069/1/vdsm.spec.in File vdsm.spec.in: Line 1605: %{python_sitelib}/yajsonrpc/stomp.py* Line 1606: %{python_sitelib}/yajsonrpc/stompReactor.py* Line 1607: Line 1608: %files infra Line 1609: %{python_sitelib}/%{vdsm_name}/infra/eventfd/__init__.py* Is it part of this patch? Line 1610: %{python_sitelib}/%{vdsm_name}/infra/filecontrol/__init__.py* Line 1611: %{python_sitelib}/%{vdsm_name}/infra/sigutils/__init__.py* Line 1612: %{python_sitelib}/%{vdsm_name}/infra/zombiereaper/__init__.py* Line 1613: %{python_sitelib}/%{vdsm_name}/infra/__init__.py* http://gerrit.ovirt.org/#/c/38069/1/vdsm/API.py File vdsm/API.py: Line 1263: message = out + err Line 1264: return {'status': {'code': rc, 'message': message}, Line 1265: 'power': 'unknown', 'operationStatus': 'initiated'} Line 1266: Line 1267: def ping(self): This verb is used by setupNetworks. I do not think that we want to change its behavior. Line 1268: "Ping the server. Useful for tests" Line 1269: updateTimestamp() Line 1270: n = self._cif.create_notification("vdsm.ping") Line 1271: n.emit(pinged=True) http://gerrit.ovirt.org/#/c/38069/1/vdsm/clientIF.py File vdsm/clientIF.py: Line 66: StompRpcServer, Line 67: ) Line 68: Line 69: Line 70: class Notification(object): This should not be part of this module Line 71: def __init__(self, event_id, cb): Line 72: self._event_id = event_id Line 73: self._cb = cb Line 74: Line 70: class Notification(object): Line 71: def __init__(self, event_id, cb): Line 72: self._event_id = event_id Line 73: self._cb = cb Line 74: why do we want to have 2 step notification sending? Line 75: def emit(self, **kwargs): Line 76: notification = json.dumps( Line 77: { Line 78: 'jsonrpc': '2.0', Line 169: def _send_notification(self, message, destination): Line 170: if self._broker_client: Line 171: self._broker_client.send(message, destination) Line 172: Line 173: self.log.warn("@@@@@") We should remove this warn. Line 174: for client in self._clients[:]: Line 175: client.send(message, destination) Line 176: Line 177: def contEIOVms(self, sdUUID, isDomainStateValid): Line 171: self._broker_client.send(message, destination) Line 172: Line 173: self.log.warn("@@@@@") Line 174: for client in self._clients[:]: Line 175: client.send(message, destination) Why do we want to send notifications to all the clients? Line 176: Line 177: def contEIOVms(self, sdUUID, isDomainStateValid): Line 178: # This method is called everytime the onDomainStateChange Line 179: # event is emitted, this event is emitted even when a domain goes http://gerrit.ovirt.org/#/c/38069/1/vdsm/protocoldetector.py File vdsm/protocoldetector.py: Line 48 Line 49 Line 50 Line 51 Line 52 do we really want to block? -- To view, visit http://gerrit.ovirt.org/38069 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id27b5ca1773139932eb5cb16921d5abec4991c5e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.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
