1
\$\begingroup\$

Please offer some advice for this code. It all works, but I know that its messy and I don't think the Exception catching is doing what I want it to. Thanks in advance for any and all advice...

def main():
    # loop over this forever
    while True:
        print '--- Getting next job for Device: %s' % device['Name']
        # get a job from the db
        try:
            job = db.JobQ.find_one({'DeviceName':device['Name'],'Status':u'ToDo'})
        except Exception as e:
            print '!!! Error getting job for Device: %s' % device['Name']
        if job:
            # if there's a job, get all the other jobs for the same file
            try:
                job_list = [
                        j for j in db.JobQ.find(
                            {'Filename':job['Filename'],'Status':u'Todo'}
                            ).sort('Created',ASCENDING)
                        ]
            except Exception as e:
                print '!!! Error getting job list for Filename: %s' % job['Filename']

            for job in job_list:
                # mark the job 'InProgress'
                job = db.JobQ.find_and_modify(
                        {'_id':ObjectId(job['_id'])},
                        {'$set':{
                            'Status':u'InProgress','Started':datetime.now()
                            }},new=True
                        )
                print '--- Performing %s on %s' % (job['Type'], job['Filename'])
                # try to do the job.. each job raises its own exceptions
                try:
                    perform_job(job)
                    job['Status'] = u'Done'
                    job['Finished'] = datetime.now()
                    db.JobQ.save(job,safe=True)
                except Exception as e:
                    # if any job fails, ditch the rest of the jobs and move on
                    print '!!! Error performing job: %s' % e
                    print 'Subsequent jobs for this filename will not run: %s'\
                            % job['Filename']
                    for job in job_list:
                        job['Finished'] = datetime.now()
                        job['Status'] = u'Failed'
                        db.JobQ.save(job,safe=True)
        else:
            print '--- No more jobs for Device: %s' % device['Name']

        sleep(device['CycleTime'])


if __name__ == '__main__':
    main()
\$\endgroup\$
2
  • \$\begingroup\$ what do you want the exception-catching to do then? \$\endgroup\$ May 23 '12 at 8:34
  • \$\begingroup\$ see half way down try: perform_job(job).. this is the section I'm not sure about. If perform_job(job) raises an Exception, the following 3 lines will not run, correct? Is this a normal way to do things? \$\endgroup\$
    – MFB
    May 23 '12 at 13:37
2
\$\begingroup\$

A couple things:

  • Instead of if job:, try if not job: ... continue . This will allow the entire (large) content of that if-block to be outdented a level, making it a little more readable.

  • Instead of job_list = [ j for j in ... ]; use job_list = list( ... ).

Also, the logic in your failure code looks a bit suspect - you set subsequent jobs as 'failed' but nothing keeps them from being attempted again by the enclosing for loop. I think you basically want a break at the end of the except block to exit the loop there, though that leaves the for loop still setting some potentially passed jobs as failed (consider the case when the first job passes but the second fails - the loop will set them all as failed). So you might instead add a fail_the_rest = False flag just before the for job in joblist loop and then as the first line of the loop do if fail_the_rest: <set failed> continue and then change the except block to just set failure on that one job and then set fail_the_rest = True. But that's all speculative due to a lack of understanding about how your job submission and pass/fails work.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the first 2 hints. My code is looking better already! And you're right. I should only be marking the job that fails and subsequent jobs as Failed whereas I'm marking them all as failed atm. I learned a lot from you response, thank you. \$\endgroup\$
    – MFB
    May 24 '12 at 0:21
  • \$\begingroup\$ In thinking about it a bit more, I realised that in a fail situation, I can check to see if each job's status is not Done and then set it Failed. Thanks for setting me on the right track! \$\endgroup\$
    – MFB
    May 24 '12 at 0:55
1
\$\begingroup\$

I think your last exception-catch needs a break statement in it - it sets all the jobs in job_list to 'Failed', but then the surrounding for job in job_list loop will still keep repeating for all the jobs, won't it? I don't know whether having the jobs maked as 'Failed' will prevent them being run, but either way something is wrong - either the 'Failed' jobs will be run, in which case there's no point marking them 'Failed', or they will not, in which case there's no point in trying to run them and you should break out of the loop.

Also, are you sure you want the jobs that have already been run to be marked 'Failed', even if they worked fine?

Another question - why is it that when you're setting marking a job as 'InProgress', you use db.JobQ.find_and_modify, but when you're marking it as 'Failed' or 'Done', you just use job['Status'] = u'Done'?

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your response. I think you're onto something (I'm just trying to get my head around it). With regards to your final question, in the first instance, I want to get and set the job and I find the job by using the only thing I have, DeviceName, but in the second instance, I only need to set it and because I have the job as it was in the db, I also have the job's _id. Pymongo allows a simple save when you have the object's _id which performs an upsert on any changed data. Hope that makes sense and thanks again! \$\endgroup\$
    – MFB
    May 24 '12 at 0:25
  • \$\begingroup\$ @MFB Oh, okay, that makes sense. I've never used Pymongo, so I wasn't sure. Good luck with the rest of your code! \$\endgroup\$
    – weronika
    May 24 '12 at 0:37
0
\$\begingroup\$

Oh I see a problem. in

 perform_job(job)
 job['Status'] = u'Done'
 job['Finished'] = datetime.now()
 db.JobQ.save(job,safe=True)

if anything goes wrong, an exception is caught by your except block. in this block, you set the status of all jobs to be Failed in another for loop. Why? It suffices to set the problematic job to be failed.

\$\endgroup\$
1
  • \$\begingroup\$ My aim with setting all the other jobs Failed if any of them fail is in a case where the first job is a copy and the second job is a delete. If the copy fails, I don't want the delete to run. Does that make sense? \$\endgroup\$
    – MFB
    May 24 '12 at 0:03

Not the answer you're looking for? Browse other questions tagged or ask your own question.