Code review comment for lp:~benji/launchpad/bug-781600

On Wed, Jul 13, 2011 at 6:13 PM, Francis J. Lacoste
<email address hidden> wrote:
> Review: Approve
> On 11-07-13 04:48 PM, Benji York wrote:
>> On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste

> Some comment on your new query:
>
>> +        query = """
>> +            SELECT unique_name, last_scanned_id, SPBDS.name FROM Branch
>> +            LEFT OUTER JOIN DistroSeries
>> +                ON Branch.distroseries = DistroSeries.id
>
> You probably want a normal join here as you don't expect any branch here
> without a distroseries. (And this join is just to get at the
> distribution right?)

Good catch. Fixed.

>> +            LEFT OUTER JOIN SeriesSourcePackageBranch
>> +                ON Branch.id = SeriesSourcePackageBranch.branch
>> +            JOIN Distribution
>> +                ON DistroSeries.distribution = Distribution.id
>> +            LEFT OUTER JOIN DistroSeries SPBDS -- (SourcePackageBranchDistroSeries)
>> +                ON SeriesSourcePackageBranch.distroseries = SPBDS.id
>> +            WHERE Distribution.name = %s""" % sqlvalues(self.name)
>
> You don't have to join the distribution here.
>
> You could use
>
> DistroSeries.distribution = self.id

Changed.

>>> It's kind of a shame that we have to materialize the whole results set here,
>>> since for huge size, it will be batched anyway by the web service code. Would
>>> it make sense to use an generator here? Maybe not, since we'd still retrieve
>>> all results for later batches.
>>
>> Given that we use ORDER BY, I suspect the only savings would be not
>> transmitting the non-batched results from the DB server to the app
>> server.  However, making the method a generator might still be a win
>> because even though they will likely come back to get the rest, that
>> request may well hit a different app server.  (I don't think we do any
>> sort of request affinity but would like to know if we do.)
>
> We don't and wouldn't really matter since each API request results in a
> different DB transaction.

Indeed. My history with ZODB is showing here. Where's the object cache
when you need it. ;)

>> Later... Nope, that didn't work.  lazr.restful exploded because it
>> couldn't adapt the generator to IFiniteSequence.  I would have expected
>> returning a generator to work, but I can't find any other instances in
>> the code base.  I've left it as a non-generator for the time being but
>> would appreciate any knowledge that can be shared on the topic.
>
> Yeah, that's fine. The direction Robert suggested might work, but not
> sure how easy/hard it would be.

Given the number of WIP items I have at the moment I really need to move
on so I'm not going to investigate further.

>>>> === modified file 'lib/lp/registry/tests/test_distro_webservice.py'
>>>
>
>>>
>>> You can use makeRelatedBranches(reference_branch=self.branch) to make it an
>>> official source package. Or use
>>> makeRelatedBranchesForSourcePackage(source_package) to create the branch and
>>> make it official in one call.
>>
>> I tried to use makeRelatedBranchesForSourcePackage and
>> makeRelatedBranches but the need to have two different distro series
>> releases (series_1 and series_2 in the original code) kept me from
>> getting it to work.  If I'm misunderstanding something, please help me.
>
> That's because those don't pass distroseries to the other factory
> methods. Would be easy to fix though.
>
> Other comments:
>
>> +        result = []
>> +        # Group on location (unique_name) and revision (last_scanned_id).
>> +        for key, group in itertools.groupby(data, itemgetter(0, 1)):
>> +            result.append(list(key))
>> +            # Pull out all the official series IDs and append them as a list
>> +            # to the end of the current record,kremoving Nones from the list.
>> +            result[-1].append(filter(None, map(itemgetter(-1), group)))
>> +        return result
>> +
>
> That comment needs fixing. Typo and you are not returning IDs anymore.

Thanks; fixed.

>> +    def test_unofficial_branch(self):
>> +        """Not all branches are official."""
>> +        # If a branch isn't official, the last skanned ID will be None and the
>> +        # official distro series list will be empty.
>> +        tips = self.lp_distro.getBranchTips()[1]
>> +        self.assertEqual(tips[0], self.unofficial_branch_name)
>> +        self.assertEqual(tips[1], None)
>> +        self.assertEqual(tips[2], [])
>
> The reason the last scanned id is None isn't because it's an unofficial
> branch, but simply because it doesn't have any revision :-)

Fixed.

Thanks for all the help.
--
Benji York