Francesco Romani has posted comments on this change. Change subject: migration: use reactor per connection ......................................................................
Patch Set 2: Code-Review-1 (4 comments) Please check my comments about self.thread. Everything else could (actually should) wait. -1 for visibility. https://gerrit.ovirt.org/#/c/59720/2//COMMIT_MSG Commit Message: Line 9: We need to make sure that reactor is not reused between incoming and Line 10: outgoing connections. Line 11: Line 12: Line 13: Change-Id: I06e21151e3e9f9e7da9e178bb0199c07f269ae8d Backport-To: ? Line 14: Signed-off-by: Piotr Kliczewski <[email protected]> https://gerrit.ovirt.org/#/c/59720/2/vdsm/clientIF.py File vdsm/clientIF.py: Line 440: Line 441: def getAllVmStats(self): Line 442: return [v.getStats() for v in self.vmContainer.values()] Line 443: Line 444: def createStompClient(self, client_socket): I understand we need this important fix, so I'm *not* pushing or requiring changes, but I think we could clean up further for master. I'll add comments as reminder for future improvements to be evaluated in later patches. Line 445: try: Line 446: from yajsonrpc.stompreactor import StompReactor Line 447: except ImportError: Line 448: raise JsonRpcBindingsError() PS2, Line 445: try: : from yajsonrpc.stompreactor import StompReactor : except ImportError: I think we should do this at top level, and use our standard idiom try: import something except ImportError: something = None # elsewhere in the module def foobar() if something is None: raise TheAppropriateException() use(something) PS2, Line 451: self.thread it is good practice to create all the instance variables in the class __init__() method. It is surprising pyflakes doesn't complain Furthermore, what happens if we have more than one migration from the same host? Each stompClient will overwrite this attribute, right? Actually I'm wondering why this needs to be an instance variable, you don't seem to use it afterwards. -- To view, visit https://gerrit.ovirt.org/59720 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06e21151e3e9f9e7da9e178bb0199c07f269ae8d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
