| StevenK | wgrant: https://code.launchpad.net/~stevenk/launchpad/3102-by-default/+merge/172937 | 01:28 |
|---|---|---|
| wgrant | StevenK: k | 01:29 |
| StevenK | wgrant: It's been weaned off -groups, was that your only concern? | 04:03 |
| wgrant | StevenK: Did you consider using Job.makeDerived instead of the stuff on line 150 of the diff? | 04:22 |
| wgrant | Also, I'd spell out SPRs in the GENERATE_PACKAGE_DIFF description, given that the initialism is ambiguous nowadays | 04:23 |
| StevenK | wgrant: I considered it, but given we're getting back an actual IJob, I didn't know what makeDerived would actually return, or just error -- and UniversalJobSource is called from another process so is difficult to debug and I know cls(job) works. | 04:24 |
| wgrant | I'm also not sure I understand exactly how the delegation from PackageDiffJobDerived works. | 04:24 |
| wgrant | StevenK: I don't think makeDerived exists on the class you want it to today, but I suggest that you make it exist. | 04:24 |
| wgrant | Make Job-only jobs as similar to non-Job-only jobs as possible | 04:25 |
| wgrant | We're just merging the additional class into Job, effectively | 04:25 |
| wgrant | So Job.makeDerived makes sense for Job-only jobs. | 04:25 |
| StevenK | Then I need a mapping of type to class and circular imports? | 04:26 |
| wgrant | A good point | 04:28 |
| wgrant | A comment to that effect would be very helpful | 04:29 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842427/ | 04:32 |
| wgrant | StevenK: That doesn't really explain anything. makeDerived not existing isn't a problem, because you could just add makeDerived. The problem is that Job.makeDerived would be a mess of circular imports, so it's probably cleaner to just do this. | 04:36 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842436/ | 04:38 |
| wgrant | That's more explanatory :) | 04:38 |
| wgrant | StevenK: I guess these can be run with run_jobs.py now? | 04:39 |
| wgrant | Rather than process-job-source-groups.py | 04:39 |
| wgrant | Hm | 04:39 |
| wgrant | process-job-source.py exists as well | 04:39 |
| wgrant | What is the different between that and run_jobs.py... | 04:39 |
| StevenK | run_jobs takes a job name, process-job-source.py takes a source | 04:41 |
| wgrant | Certainly | 04:41 |
| wgrant | But what is the difference? | 04:41 |
| wgrant | AFAICT run_jobs shouldn't exist any more | 04:42 |
| wgrant | There are still three config sections for it | 04:42 |
| StevenK | process-job-source.py uses the same internals as run_jobs, and the latter is still being used on ackee and loganberry | 04:42 |
| wgrant | But I suspect they can be migrated to process-job-source.py-style entries in about 20 lines of diff | 04:42 |
| wgrant | How odd | 04:43 |
| StevenK | It's backing of JobCronScript can't die, however | 04:43 |
| StevenK | % bzr grep run_jobs | cut -d/ -f8 | 04:43 |
| StevenK | run_jobs.py -vv initializedistroseries >> | 04:43 |
| StevenK | run_jobs.py pofile_stats >> | 04:43 |
| StevenK | run_jobs.py packaging_translations -q --log-file=INFO: | 04:43 |
| wgrant | Why not? | 04:43 |
| StevenK | JobCronScript is used by process-job-source.py, scan_branches and a bunch of others. | 04:44 |
| wgrant | scan_branches etc. are irrelevant | 04:44 |
| wgrant | All that matters is process-job-source.py | 04:44 |
| wgrant | And it would become the One True Runner | 04:44 |
| wgrant | So JobCronScript would become a part of it | 04:44 |
| wgrant | distroseriesdifference_job, merge-proposal-jobs, process-apport-blobs etc die | 04:45 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842443/ | 04:45 |
| StevenK | Ah, you're there. | 04:45 |
| StevenK | wgrant: Happy to start digging the graves for those scripts in a seperate branch. | 04:46 |
| wgrant | I'm doing that :) | 04:46 |
| StevenK | Ah | 04:46 |
| wgrant | So we'll be down to just process-job-source.py and celery as job runners. | 04:46 |
| StevenK | And it will be glorious | 04:46 |
| wgrant | (process-job-source-groups just launches multiple process-job-sources) | 04:47 |
| StevenK | wgrant: You do seem to have been distracted quite heavily from reviewing my branch, though. :-) | 04:49 |
| wgrant | StevenK: I was hoping you'd checked that process-job-source actually manages to run the job | 04:49 |
| StevenK | wgrant: As in IPackageDiffJob ? | 04:50 |
| wgrant | Yes | 04:50 |
| wgrant | Ah | 04:50 |
| wgrant | There is a test, I see | 04:50 |
| StevenK | Yes | 04:50 |
| StevenK | I don't need to check, I wrote a test to check for me. | 04:50 |
| wgrant | StevenK: Can you explain how the delegation for PackageDiffJobDerived works? | 04:51 |
| wgrant | self.context = self is slightly smelly | 04:51 |
| StevenK | self.context = self is for the benefit of celery who assumes that context is a job class | 04:52 |
| wgrant | Isn't that for the delegates()? | 04:52 |
| StevenK | No, that's self.job | 04:52 |
| StevenK | job is for delegates, context is for celery | 04:53 |
| wgrant | How does delegate(IPackageDiffJob) know to look at self.job rather than self.context? | 04:53 |
| wgrant | Also, what's that __storm_table__ doing there? | 04:53 |
| StevenK | __storm_table__ is there because celery calls IStore(db_class) | 04:54 |
| wgrant | Right, but db_class should be the Job in this case | 04:54 |
| StevenK | With no __storm_table__, we can't adapt | 04:54 |
| wgrant | Sure | 04:54 |
| StevenK | Because the IStore adapter looks up a table | 04:54 |
| wgrant | Because it's not a DB class. | 04:54 |
| wgrant | So celery shouldn't be trying to use it as one | 04:54 |
| wgrant | The DB class is Job | 04:55 |
| StevenK | Right, but it got handed a PackageDiffJob | 04:55 |
| StevenK | It assumes all jobs it gets are DB backed | 04:55 |
| wgrant | That's not a valid assumption | 04:55 |
| wgrant | It could perhaps call a getDBObject method | 04:55 |
| wgrant | We should fix the celery integration rather than faking a DB class. | 04:55 |
| wgrant | Since the celery integration is probably actually easier to change | 04:56 |
| StevenK | getDBObject? | 04:57 |
| wgrant | A method on a job that returns the DB object. | 04:57 |
| wgrant | It's an example of a way to fix this that isn't evil. | 04:57 |
| StevenK | I didn't think __storm_table__ wasn't so evil ... it's correct in one sense | 04:58 |
| wgrant | It's not | 04:58 |
| StevenK | It is, we have a row in the Job table about it., | 04:59 |
| wgrant | That's a very low threshold for correctness :) | 04:59 |
| wgrant | The job system has enough hacks that we haven't removed yet; we should do the small amount of work needed to integrate this nicely, rather than piling on more. Particularly when the additional hacks have the side-effect of turning a normal object into a really strange faux-Storm object with consequences that I haven't fully determined yet. | 05:01 |
| StevenK | wgrant: Right, so we want it to be a classmethod that returns IStore(cls) for the base class, and PackageDiffJob can return IStore(Job) ? | 05:03 |
| StevenK | getStore() or something | 05:04 |
| wgrant | StevenK: Um, possibly, I forget exactly how this is structured at the moment. | 05:04 |
| wgrant | StevenK: How does it work for non-Job-only jobs? | 05:04 |
| wgrant | eg. BranchJobDerived isn't a Storm object | 05:05 |
| wgrant | It still just delegates to BranchJob | 05:05 |
| StevenK | db_class in this case is BranchJob | 05:05 |
| StevenK | Or so | 05:05 |
| wgrant | Sure | 05:05 |
| wgrant | But how does it get there? | 05:05 |
| wgrant | Oh | 05:05 |
| wgrant | Because celery is given a key of BranchJob? | 05:06 |
| wgrant | Not the concrete enumerated class? | 05:06 |
| StevenK | Hm | 05:06 |
| StevenK | The job classes themselves are db classes | 05:06 |
| StevenK | I thought they had a generic base class | 05:06 |
| StevenK | So PackageDiffJob is the odd one out | 05:07 |
| wgrant | Not really, no | 05:07 |
| wgrant | BranchScanJob is a BranchJobDerived | 05:07 |
| wgrant | So BranchScanJob isn't a DB class; it just delegates to BranchJob, which is one. | 05:07 |
| wgrant | PackageDiffJob is the same; it just delegates to a Job, which is one. | 05:07 |
| StevenK | SharingJob for instance, is StormBase | 05:07 |
| wgrant | SharingJob isn't equivalent to PackageDiffJob | 05:08 |
| wgrant | SharingJob is the DB class that underlies the concrete sharing jobs | 05:08 |
| StevenK | Right | 05:08 |
| wgrant | lib/lp/registry/model/sharingjob.py:class RemoveArtifactSubscriptionsJob(SharingJobDerived): | 05:08 |
| StevenK | It backs onto SharingJobDerived, which is backed onto BaseRunnableJob | 05:08 |
| wgrant | Exactlyu | 05:09 |
| wgrant | The actual job classes are never Storm objects | 05:09 |
| wgrant | Which is why it's completely wrong that PackageDiffJob is | 05:09 |
| wgrant | Which is the line in celery that expects the job to be storm? | 05:09 |
| wgrant | I suspect it's just before it uses UniversalJobSource | 05:09 |
| StevenK | It's in UniversalJobSource | 05:09 |
| wgrant | The difference is the lack of makeDerived. | 05:09 |
| wgrant | For traditional jobs, the key is ('BranchJob', id) | 05:10 |
| wgrant | IStore(BranchJob) works | 05:10 |
| StevenK | store = IStore(db_class) in UniversalJobSource.get | 05:10 |
| wgrant | And the UniversalJobSource looks up the BranchJob by id | 05:10 |
| wgrant | Oh! | 05:10 |
| wgrant | That makes more sense :) | 05:10 |
| wgrant | So it's not in the celery stuff | 05:10 |
| StevenK | It's called by the celery stuff | 05:10 |
| wgrant | Yeah | 05:11 |
| wgrant | But I was wondering how celery could see this | 05:11 |
| wgrant | Given that the bits that could break were in UJS | 05:11 |
| wgrant | So | 05:11 |
| wgrant | Easy fix | 05:11 |
| wgrant | If db_class isn't actually a DB class, assume Job | 05:11 |
| wgrant | So UJS can take two different things | 05:11 |
| wgrant | 1) the traditional intermediate job DB class, like BranchJob or SharingJob, plus a Job.id | 05:12 |
| wgrant | 2) a Job-only job class, like PackageDiffJob, plus a Job.id | 05:12 |
| StevenK | wgrant: But __storm_table__ == 'Job | 05:12 |
| wgrant | In 1) it uses BranchJob.makeDerived to get the concrete one | 05:12 |
| StevenK | Is how we do return db_class(db_job) | 05:12 |
| wgrant | StevenK: Only because you put it there to placate the code that we're replacing | 05:12 |
| wgrant | What do you mean? | 05:13 |
| wgrant | (also, db_class isn't a DB class in case 2, so it should probably be renamed) | 05:13 |
| StevenK | wgrant: IOW, I'm using __storm__table__ == 'Job' to work out if it's a fake job | 05:13 |
| wgrant | StevenK: Yes | 05:13 |
| wgrant | And instead you're going to work out if it's a Job-only (aka. fake) job by the *lack* of __storm_table__ | 05:13 |
| StevenK | Righ | 05:14 |
| StevenK | *Right | 05:14 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842508/ | 05:32 |
| wgrant | StevenK: Not fake, but represented in the database by only a Job row. | 05:35 |
| wgrant | The two different inputs that the method takes are quite confusing to distinguish, so I'd suggest a few lines of comment there describing the two cases | 05:35 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842515/ | 05:35 |
| wgrant | A good start | 05:38 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842531/ | 05:41 |
| wgrant | StevenK: Heh, that doesn't quite describe the extraordinary oddness | 05:43 |
| wgrant | In the Job-only case, the class name we are given is the concrete job | 05:43 |
| wgrant | In the non-Job-only case, the class name we are given is the intermediate abstract job class | 05:43 |
| StevenK | # - Jobs that have no backing, and are only represented by a row in | 05:44 |
| StevenK | # the Job table, but the class name we are given is the abstract | 05:44 |
| StevenK | # job class. | 05:44 |
| wgrant | Sounds good | 05:45 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842539/ is the diff | 05:46 |
| wgrant | StevenK: One more thing I've just thought of... it possibly makes sense in PDJ.__init__ to assert that job.job_type matches what we expect | 05:47 |
| wgrant | Just in case anything funny happens and there's eg. an ID mixup due to a celery issue | 05:47 |
| StevenK | wgrant: Hm, we don't ignore FAILED diffs -- I don't think we can deal with that iterReady, so I guess we should have run() just return None if status != PENDING | 05:51 |
| wgrant | StevenK: Didn't we do something to handle that a couple of weeks ago? | 05:52 |
| wgrant | Not running jobs unless they are runnable | 05:52 |
| wgrant | Or do we in this case have a WAITING job for a FAILED diff? | 05:53 |
| StevenK | wgrant: The job *itself* will be runnable, some diffs will be created as FAILED -- eg: udev | 05:53 |
| wgrant | That sounds like a bug | 05:53 |
| wgrant | There should be no job in that case. | 05:53 |
| StevenK | wgrant: http://pastebin.ubuntu.com/5842550/ | 05:56 |
| wgrant | StevenK: Right | 05:57 |
| StevenK | I'll be happy when that diff bug is fixed and we can rip that rubbish out | 05:58 |
| StevenK | wgrant: Any other concerns? | 05:58 |
| wgrant | I don't think so | 05:59 |
| === tasdomas_afk is now known as tasdomas | ||
| StevenK | wgrant: Did you forget to remove all of the obsoleted cronscripts? | 09:44 |
| wgrant | StevenK: In a second branch | 10:06 |
| wgrant | StevenK: So we don't break everything ever when this rev is deployed | 10:06 |
| StevenK | Not everything ever, just most jobs. :-) | 10:07 |
| === tasdomas is now known as tasdomas_afk | ||
| === tasdomas_afk is now known as tasdomas | ||
| === tasdomas is now known as tasdomas_afk | ||
| xnox | Why is https://launchpad.net/~costamagnagianfranco/+archive/firefox "firefox-backports" ?! building boinc on virt-arm ?! | 15:48 |
| === _mup__ is now known as _mup_ | ||
| === s1aden is now known as sladen | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!