Martin Polednik has posted comments on this change. Change subject: virt: add VmTracker to track domain traffic ......................................................................
Patch Set 19: Code-Review+1 (3 comments) nothing major spotted and I trust your reasoning behind the code, treat comments as opinions. https://gerrit.ovirt.org/#/c/44543/19/tests/vmUtilsTests.py File tests/vmUtilsTests.py: Line 160: Line 161: def test_busy_until_cancelation(self): Line 162: self.tracker.reserve_available(self.vms) Line 163: # now vms previously available are reserved Line 164: self.assertFalse(self.tracker.reserve_available(self.vms)) (opinionated) 'assertEqual' and 'None' delivers delivers the information easier :) Line 165: Line 166: def test_available_after_use(self): Line 167: for vm_id in self.tracker.reserve_available(self.vms): Line 168: with self.tracker.run_on(vm_id): https://gerrit.ovirt.org/#/c/44543/19/vdsm/virt/utils.py File vdsm/virt/utils.py: Line 119: rmFile(os.path.realpath(sock)) Line 120: rmFile(sock) Line 121: Line 122: Line 123: # TODO: horrible unexpressive name. Find a better one. inexpressive* :) Line 124: class VmTracker(object): Line 125: Line 126: def __init__(self): Line 127: self._lock = threading.Lock() Line 120: rmFile(sock) Line 121: Line 122: Line 123: # TODO: horrible unexpressive name. Find a better one. Line 124: class VmTracker(object): actually not *that* bad, if you don't mind the longer name then VmAvailabilityTracker (and possibly reserve_available -> reserve?) is quite expressive Line 125: Line 126: def __init__(self): Line 127: self._lock = threading.Lock() Line 128: self._busy = set() -- To view, visit https://gerrit.ovirt.org/44543 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iab735193ea7b2e177e146099843cc31706379e68 Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
