Code review comment for lp:~al-maisan/launchpad/pending-jobs-499861

On Thu, Dec 24, 2009 at 8:16 PM, Muharem Hrnjadovic <email address hidden> wrote:
> Jonathan Lange wrote:
>> On Thu, Dec 24, 2009 at 3:45 AM, Muharem Hrnjadovic
>> <email address hidden> wrote:
>>> Muharem Hrnjadovic has proposed merging lp:~al-maisan/launchpad/pending-jobs-499861 into lp:launchpad/devel.
>>>
>>>    Requested reviews:
>>>    Canonical Launchpad Engineering (launchpad)
>>> Related bugs:
>>>  #499861 Define interface allowing build farm job classes to select pending jobs
>>>  https://bugs.launchpad.net/bugs/499861
>>>
>>>
>>> Hi there!
>>>
>>> this branch
>>>
>>>    - introduces a method (IBuildFarmJob.pendingJobsQuery()) that all
>>>      Soyuz build farm jobs need to implement in order to enable a
>>>      general (i.e. across all job types) job dispatch time estimation.
>>
>> Although this sounds like a good idea to me, I'm confused by the
>> terminology. Why do we ask a job (singular) to return a list of
>> pending jobs (plural)? It seems like this method would be better
>> placed on an interface representing a type of job.
>
> Hmm .. good point .. can you point to an example?
>
> The original idea was that these methods be implemented as static
> methods. I can stress that point in the doc string if desired.
>

I don't know examples off the top of my head, but I reckon if you grep
for classProvides, you'll find some fairly quickly. The key point is
that static methods are a different interface.

>>> +        :param virtualized: the job of interest (JOI) can only run
>>> +            on builders with this virtualization setting.
>>
>> If I understand correctly, when this is passed in, it means that we're
>> telling the job that we intend to run it with a particular
>> virtualization setting. The way it's phrased in the docstring as-is
>> seems a bit backwards, but I can't think of how to phrase it better.
>> Can you?
> I did try :) see how you like the new doc string.
>

It's great, thanks.

>>> +            Again, this information can be used to narrow down the
>>> +            pending jobs that will result from the returned query and
>>> +            processor independent job types may safely ignore it.
>>> +        :return: a string SELECT clause that can be used to find
>>> +            the pending jobs of the appropriate type.
>>
>> Here's my big question: why a string?
>>
>> Why not return a Storm ResultSet or a Select object?
> Good question, I tried both a Storm ResultSet and a Select object (took
> me a day at least).
>
> Ultimately it did not work because
>
>  - once other job types are added to the build farm I will need to
>    UNION their queries and then *order* the UNIONed result set
>  - the job types that do not care about processor or virtualization
>    will return None/NULL "values" for these columns in their result
>    sets
>  - neither the storm `Union` facility nor the ResultSet.union() method
>    would support ordering of result sets that have *literal* values.
>
> I did also ask on the #storm IRC channel but did not get any answers
> that seemed to fit my problem.
>

Hmm. In that case, keep what you've got.

It might be worth asking on the storm mailing list for interests sake,
or future work.

>>> +    virtualized = Attribute(
>>> +        _(
>>> +            "The virtualization setting required by this build farm job. "
>>> +            "For job types that do not care about virtualization please "
>>> +            "return None."))
>>>
>>
>> You don't mention either of these new attributes in your cover letter
>> -- what's their purpose?
> When interrogated each build farm job shall be able to tell what its
> processor/virtualization requirements are.
>
> "Build a source package from recipe" type jobs would e.g. return None
> for both thus indicating that they (do not care about processor or
> virtualization and) may run on any builder.
>

Ahh OK, so these properties are on the objects rather than on the
class? That makes sense.

...
>> More generally, surely there have to be very, very similar tests
>> already in Launchpad to test the build queue behaviour. Or am I
>> missing something?
> These existing build dispatch time estimation tests barely scratch the
> surface unfortunately.
>

In that case, I'm glad you're adding these :)

Please look into using classProvides before landing this patch. I
won't be able to do a follow up, but would be perfectly happy with you
finding someone else to do the last rubber stamp.

jml