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