Nir Soffer has posted comments on this change.

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


Patch Set 22:

(1 comment)

http://gerrit.ovirt.org/#/c/26300/22/vdsm/protocolDetector.py
File vdsm/protocolDetector.py:

Line 201:     required_size = 7
Line 202: 
Line 203:     def __init__(self, json_binding):
Line 204:         self.json_binding = json_binding
Line 205:         self.reactor = self.json_binding.createStompReactor()
> Initial design was that there is an instance of reactor per detector. There
Ok, it should work in the way you describe, but still letting the detector 
manage the reactor looks strange, and does not belong in this module.

If you put the detector in the jsonrpc module, I would not complain that it 
knows about the reactor and json rpc internals. For this module, a detector is 
something that expose the detection interface (require_size, detect, 
handleSocket, stop).
Line 206:         self.json_binding.startReactor(self.reactor)
Line 207: 
Line 208:     def detect(self, data):
Line 209:         return data.startswith("CONNECT") or data.startswith("SEND")


-- 
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to