Change in vdsm[master]: migration: Add incoming migration semaphore
fromani at redhat.com
fromani at redhat.com
Tue Oct 6 16:15:04 UTC 2015
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 <mbetak at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjelinek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list