Francesco Romani has posted comments on this change. Change subject: migration: Add retry on full capacity ......................................................................
Patch Set 18: Code-Review+1 (3 comments) Looks good, but a couple of questions inside. Overall, -1 for visibility https://gerrit.ovirt.org/#/c/52799/18/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 334: 'migCancelErr', message='Migration canceled') Line 335: raise Line 336: except MigrationLimitExceeded: Line 337: self.log.debug("Migration destination busy. Initiating " Line 338: "retry in %d seconds." % retry_timeout) works. Better: self.log.debug("Migration destination busy. Initiating " "retry in %d seconds.", retry_timeout) This is because python logging module will do the string formatting if and only if the message is going to be logged. Otherwise, the string formatting will always happen, and this is (indeed a [tiny] bit) wasteful. Line 339: time.sleep(retry_timeout) Line 340: except MigrationDestinationSetupError as e: Line 341: self._recover(str(e)) Line 342: # we know what happened, no need to dump hollow stack trace Line 370: destCreationTime) Line 371: Line 372: if response.is_error(result): Line 373: self.status = result Line 374: # Differentiate between general migration failure and a retry I don't think this comment is worth keeping. Which value does it add? Line 375: if is_retry_error(result): Line 376: raise MigrationLimitExceeded() Line 377: else: Line 378: raise MigrationDestinationSetupError( Line 601: self._vm.log.debug('stopping migration monitor thread') Line 602: self._stop.set() Line 603: Line 604: Line 605: def is_retry_error(result): I don't really see the point of this helper. Could you please elaborate why it is better to have it? Btw, this is minor, I don't really mind that much. -- To view, visit https://gerrit.ovirt.org/52799 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I988fa2e501eb77d121668b22cc533b744a3dc755 Gerrit-PatchSet: 18 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 Betak <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
