On Wed, Jun 23, 2010 at 10:30 AM, Iustin Pop <[email protected]> wrote: > On Wed, Jun 23, 2010 at 10:27:45AM +0200, Guido Trotter wrote: >> We move from querying the in-memory version to loading all jobs from the >> disk. Since the jobs are written/deleted on disk in an atomic manner, we >> don't need to lock at all. Also, since we're just looking at the >> contents of a directory, we don't need to check that the job queue is >> "open". >> >> If some jobs are removed between when we listed them and us loading >> them, we need to be able to cope: if we were asked to load those jobs >> specifically, we must report the failure, but if we were just asked to >> "load all" we shall just not consider them as part of the "all" set, >> since they were deleted. >> >> Signed-off-by: Guido Trotter <[email protected]> >> --- >> lib/jqueue.py | 35 +++++++++++------------------------ >> 1 files changed, 11 insertions(+), 24 deletions(-) >> >> diff --git a/lib/jqueue.py b/lib/jqueue.py >> index d8249df..1165ad4 100644 >> --- a/lib/jqueue.py >> +++ b/lib/jqueue.py >> @@ -1031,21 +1031,6 @@ class JobQueue(object): >> >> return job >> >> - def _GetJobsUnlocked(self, job_ids): >> - """Return a list of jobs based on their IDs. >> - >> - �...@type job_ids: list >> - �...@param job_ids: either an empty list (meaning all jobs), >> - or a list of job IDs >> - �...@rtype: list >> - �...@return: the list of job objects >> - >> - """ >> - if not job_ids: >> - job_ids = self._GetJobIDsUnlocked() >> - >> - return [self._LoadJobUnlocked(job_id) for job_id in job_ids] >> - >> def SafeLoadJobFromDisk(self, job_id): >> """Load the given job file from disk. >> >> @@ -1426,14 +1411,9 @@ class JobQueue(object): >> >> return (archived_count, len(all_job_ids) - last_touched) >> >> - �[email protected] >> - �...@_requireopenqueue >> def QueryJobs(self, job_ids, fields): >> """Returns a list of jobs in queue. >> >> - This is a wrapper of L{_GetJobsUnlocked}, which actually does the >> - processing for each job. >> - >> �...@type job_ids: list >> �...@param job_ids: sequence of job identifiers or None for all >> �...@type fields: list >> @@ -1444,12 +1424,19 @@ class JobQueue(object): >> >> """ >> jobs = [] >> + list_all = False >> + if not job_ids: >> + # Since files are added to/removed from the queue atomically, there's >> no >> + # risk of getting the job ids in an inconsistent state. >> + job_ids = self._GetJobIDsUnlocked() >> + list_all = True >> >> - for job in self._GetJobsUnlocked(job_ids): >> - if job is None: >> - jobs.append(None) >> - else: >> + for job_id in job_ids: >> + job = self.SafeLoadJobFromDisk(job_id) >> + if job is not None: >> jobs.append(job.GetInfo(fields)) >> + elif not list_all: >> + jobs.append(None) >> >> return jobs > > I still don't see how you report the failure here… you're just not > adding 'None', i.e. making the list smaller, which will probably break > the clients. >
No, it's the other way around. Adding "None" breaks the client. (counteintuitive, I know). Currently the client expects a list of (any number of) non-broken jobs, if it wanted all jobs, while it expects a list of the same size of the request if it wanted certain specific jobs. If it gets a None and it didn't ask for specific job numbers it stack-traces, while of course it wants the None if it wanted a certain job and it's not found. So in the current version we're not reporting the error, but in the future we should fix the client and server to do better error reporting throughout. Thanks, Guido
