Code review comment for lp:~al-maisan/launchpad/spec-job-class-503839

Michael Nelson wrote:
> Review: Needs Information code
[..]
> I've got one rather large question about this change: Why can't you
> just use a normal adapter? It seems that you've gone the long way
> around to implement adapter-like functionality.
>
> That is, you've got a BuildFarmJobType specified on
> IBuildQueue.job_type, and you want to find the corresponding class. I
> don't know whether it is the best solution, but you *could* in this
> case, adapt the build_queue entry itself to the correct IBuildFarmJob
> implementation?
>
> IBuildFarmJob(specific_build_queue_entry)
How would that actually work? IBuildFarmJob is not what we want here.
IBuildQueue.specific_job is supposed to yield something like a
BuildPackageJob instance.

> which would return the specifc build farm job.
>
> I'm wondering if it's really that you need specific_job_classes for
> further work in the build estimations, rather than specifically for
That's the case indeed, sorry for not explaining that better.

> this branch. That is, originally when you were chatting with gary
> about using the class manager, I had understood that you needed to
> *iterate* over the different classes to build a query... in that case
> you would certainly need something like the specific_job_classes that
> you've provided below. But as it is, you are not needing to iterate
> them at all for this branch, and looking at this branch in isolation
> would suggest that a normal adapter would be more intuitive.
Sorry for not explaining in more detail why this is needed please see my
previous email for an example where I do need to iterate over the build
farm job classes.
I also expect that that's something that will be needed when we
generalize the candidate job selection.

> If it is the case that you will need specific_job_classes for a later
> branch, then it may indeed make sense to *not* use adaption here and
> re-use the specific_job_classes exactly as you've done, as otherwise
As stated in the previous reply I do require the iteration over the build
farm job classes for the purpose of job dispatch time estimation.
Being able to register build farm classes in the way suggested makes it
also much easier to test the code.
Please see the end of the diff (lines 193-222) enclosed in my previous
reply for an example.

[..]

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC