Francesco Romani has posted comments on this change. Change subject: migration: Add incoming migration semaphore ......................................................................
Patch Set 8: Code-Review-1 (4 comments) Good improvements! the throttle decorator is very nice. Few comments inside, -1 for visibility https://gerrit.ovirt.org/#/c/45954/8//COMMIT_MSG Commit Message: Line 7: migration: Add incoming migration semaphore Line 8: Line 9: Added throttling of incoming migrations. Line 10: Line 11: 'migrationCreate' werb can now return new error status to signify full to report full occupation? Line 12: occupation. Line 13: Line 14: Wiki: http://www.ovirt.org/Features/Migration_Enhancements Line 15: Change-Id: I8952f732033ed160292b11fbc0c4deac099b2b3e https://gerrit.ovirt.org/#/c/45954/8/lib/vdsm/response.py File lib/vdsm/response.py: Line 69: return code != doneCode["code"] Line 70: Line 71: Line 72: def is_success(res): Line 73: return not is_error(res) please just use if not response.is_error(res) https://gerrit.ovirt.org/#/c/45954/8/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1277: else: Line 1278: yield Line 1279: Line 1280: Line 1281: def throttle(semaphore, err, blocking): brilliant! But please split to a separate patch, and make sure to add tests to cover all the major flows. Furthermore, this should probably be introduced in vdsm/virt/utils.py, and only after moved in the general utils. At first glance looks fine, will review later. Line 1282: """ Line 1283: Helper decorator for API methods that should have limited number Line 1284: of concurrent executions. Line 1285: https://gerrit.ovirt.org/#/c/45954/8/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2777: del self.conf['username'] Line 2778: self.saveState() Line 2779: self.log.info("End of migration") Line 2780: Line 2781: def _checkMigrationCompletedDuringRecovery(self): nice cleanup (although not 100% happy about the name) but please move it into a separate patch. Line 2782: if not self.recovering: Line 2783: return False Line 2784: Line 2785: try: -- To view, visit https://gerrit.ovirt.org/45954 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8952f732033ed160292b11fbc0c4deac099b2b3e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
