Nir Soffer has posted comments on this change. Change subject: jobs: Fix abort semantics ......................................................................
Patch Set 3: Code-Review+1 (7 comments) Nice and simple! Please check the comments. https://gerrit.ovirt.org/#/c/65102/3/lib/vdsm/jobs.py File lib/vdsm/jobs.py: Line 168 Line 169 Line 170 Line 171 Line 172 Should raise? Line 169 Line 170 Line 171 Line 172 Line 173 We must remove this line, _abort cannot know if the job was aborted, it is async. Line 152: return Line 153: try: Line 154: self._run() Line 155: except exception.ActionStopped: Line 156: self._handle_completed_abort() Maybe _abort_completed? Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state Line 154: self._run() Line 155: except exception.ActionStopped: Line 156: self._handle_completed_abort() Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Maybe _run_failed? Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state Line 161: # _run might finish normally before the abort action executes. Line 162: with self._status_lock: Line 157: except Exception as e: Line 158: self._handle_execution_error(e) Line 159: else: Line 160: # We always mark the job done. Even if we were in aborting state Line 161: # _run might finish normally before the abort action executes. I think this comment is confusing, setting status to DONE in the else is pretty clear to me. Line 162: with self._status_lock: Line 163: self._status = STATUS.DONE Line 164: finally: Line 165: if self.autodelete: Line 162: with self._status_lock: Line 163: self._status = STATUS.DONE Line 164: finally: Line 165: if self.autodelete: Line 166: self._autodelete() Cannot be more elegant! Line 167: Line 168: def _prepare_to_run(self): Line 169: with self._status_lock: Line 170: if self.status == STATUS.ABORTED: Line 164: finally: Line 165: if self.autodelete: Line 166: self._autodelete() Line 167: Line 168: def _prepare_to_run(self): May be _may_run? Line 169: with self._status_lock: Line 170: if self.status == STATUS.ABORTED: Line 171: logging.debug('Refusing to run aborted job %r', self._id) Line 172: return False -- To view, visit https://gerrit.ovirt.org/65102 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I801082c50b10cf0571210d65cd3a5cec0d282a5c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
