On Tue, Dec 16, 2008 at 3:42 PM, Michael Hanselmann <han...@google.com> wrote:
>
> This is done by passing the job object to _ArchiveJobUnlocked instead
> of only the job ID. Also return whether job was actually archived.
> ---
> lib/jqueue.py | 38 ++++++++++++++++++++------------------
> 1 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/lib/jqueue.py b/lib/jqueue.py
> index e347485..45ff412 100644
> --- a/lib/jqueue.py
> +++ b/lib/jqueue.py
> @@ -1111,25 +1111,18 @@ class JobQueue(object):
> self.UpdateJobUnlocked(job)
>
> @_RequireOpenQueue
> - def _ArchiveJobUnlocked(self, job_id):
> + def _ArchiveJobUnlocked(self, job):
> """Archives a job.
>
> - @type job_id: string
> - @param job_id: the ID of job to be archived
> + @type job: L{_QueuedJob}
> + @param job: Job object
>
can you add the @return here?
> """
> - logging.info("Archiving job %s", job_id)
> -
> - job = self._LoadJobUnlocked(job_id)
> - if not job:
> - logging.debug("Job %s not found", job_id)
> - return
> -
> if job.CalcStatus() not in (constants.JOB_STATUS_CANCELED,
> constants.JOB_STATUS_SUCCESS,
> constants.JOB_STATUS_ERROR):
> logging.debug("Job %s is not yet done", job.id)
> - return
> + return False
>
> old = self._GetJobPath(job.id)
> new = self._GetArchivedJobPath(job.id)
> @@ -1138,6 +1131,8 @@ class JobQueue(object):
>
> logging.debug("Successfully archived job %s", job.id)
>
> + return True
> +
> @utils.LockedMethod
> @_RequireOpenQueue
> def ArchiveJob(self, job_id):
> @@ -1149,7 +1144,14 @@ class JobQueue(object):
> @param job_id: Job ID of job to be archived.
>
> """
> - return self._ArchiveJobUnlocked(job_id)
> + logging.info("Archiving job %s", job_id)
> +
> + job = self._LoadJobUnlocked(job_id)
> + if not job:
> + logging.debug("Job %s not found", job_id)
> + return
return None, perhaps?? Can you also document what this function returns.
> +
> + return self._ArchiveJobUnlocked(job)
>
> @utils.LockedMethod
> @_RequireOpenQueue
> @@ -1168,12 +1170,12 @@ class JobQueue(object):
> logging.info("Archiving jobs with age more than %s seconds", age)
>
> now = time.time()
> - for jid in self._GetJobIDsUnlocked(archived=False):
> - job = self._LoadJobUnlocked(jid)
> - if job.CalcStatus() not in (constants.OP_STATUS_SUCCESS,
> - constants.OP_STATUS_ERROR,
> - constants.OP_STATUS_CANCELED):
> + for job_id in self._GetJobIDsUnlocked(archived=False):
> + # Returns None if the job failed to load
> + job = self._LoadJobUnlocked(job_id)
> + if not job:
> continue
> +
> if job.end_timestamp is None:
> if job.start_timestamp is None:
> job_age = job.received_timestamp
> @@ -1183,7 +1185,7 @@ class JobQueue(object):
> job_age = job.end_timestamp
>
> if age == -1 or now - job_age[0] > age:
> - self._ArchiveJobUnlocked(jid)
> + self._ArchiveJobUnlocked(job)
>
LGTM for the rest! :)
Thanks,
Guido
