[01:28] <StevenK> wgrant: https://code.launchpad.net/~stevenk/launchpad/3102-by-default/+merge/172937
[01:29] <wgrant> StevenK: k
[04:03] <StevenK> wgrant: It's been weaned off -groups, was that your only concern?
[04:22] <wgrant> StevenK: Did you consider using Job.makeDerived instead of the stuff on line 150 of the diff?
[04:23] <wgrant> Also, I'd spell out SPRs in the GENERATE_PACKAGE_DIFF description, given that the initialism is ambiguous nowadays
[04:24] <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:25] <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:26] <StevenK> Then I need a mapping of type to class and circular imports?
[04:28] <wgrant> A good point
[04:29] <wgrant> A comment to that effect would be very helpful
[04:32] <StevenK> wgrant: http://pastebin.ubuntu.com/5842427/
[04:36] <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:38] <StevenK> wgrant: http://pastebin.ubuntu.com/5842436/
[04:38] <wgrant> That's more explanatory :)
[04:39] <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:41] <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:42] <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:43] <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:44] <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:45] <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:46] <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:47] <wgrant> (process-job-source-groups just launches multiple process-job-sources)
[04:49] <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:50] <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:51] <wgrant> StevenK: Can you explain how the delegation for PackageDiffJobDerived works?
[04:51] <wgrant> self.context = self is slightly smelly
[04:52] <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:53] <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:54] <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:55] <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:56] <wgrant> Since the celery integration is probably actually easier to change
[04:57] <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:58] <StevenK> I didn't think __storm_table__ wasn't so evil ... it's correct in one sense
[04:58] <wgrant> It's not
[04:59] <StevenK> It is, we have a row in the Job table about it.,
[04:59] <wgrant> That's a very low threshold for correctness :)
[05:01] <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:03] <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:04] <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:05] <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:06] <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:07] <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:08] <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:09] <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:10] <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:11] <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:12] <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:13] <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:14] <StevenK> Righ
[05:14] <StevenK> *Right
[05:32] <StevenK> wgrant: http://pastebin.ubuntu.com/5842508/
[05:35] <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:38] <wgrant> A good start
[05:41] <StevenK> wgrant: http://pastebin.ubuntu.com/5842531/
[05:43] <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:44] <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:45] <wgrant> Sounds good
[05:46] <StevenK> wgrant: http://pastebin.ubuntu.com/5842539/ is the diff
[05:47] <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:51] <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:52] <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:53] <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:56] <StevenK> wgrant: http://pastebin.ubuntu.com/5842550/
[05:57] <wgrant> StevenK: Right
[05:58] <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:59] <wgrant> I don't think so
[09:44] <StevenK> wgrant: Did you forget to remove all of the obsoleted cronscripts?
[10:06] <wgrant> StevenK: In a second branch
[10:06] <wgrant> StevenK: So we don't break everything ever when this rev is deployed
[10:07] <StevenK> Not everything ever, just most jobs. :-)
[15:48] <xnox> Why is https://launchpad.net/~costamagnagianfranco/+archive/firefox "firefox-backports" ?! building boinc on virt-arm ?!