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

On 11-07-13 04:48 PM, Benji York wrote:
> On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste

>
>> You probably need to make since=None since it's not a required parameter.
>
> It seems odd to me to put the default in the interface in addition to in
> the implementation. It has no effect and I worry that the DRY violation
> could result in a loss of synchronization with the real default and
> result in confusion. Also, the "required=False" bit is what indicates
> that "since" is optional, the interface reader shouldn't care what
> internal value we use to signify that it wasn't provided.
>
> That being said, I'm only -0 so I'll add it given a bit of a push.

Well, interfaces and implementation are usually not on the side of DRY
:-) To me, if the interface doesn't have a default parameter, it usually
means that client should expect to have to provide a parameter. Again,
the fact that this is mainly for webservice consumption makes this a
little less important. Anyway, Do as you will!

>
>>> === modified file 'lib/lp/registry/model/distribution.py'
>>
>>> """See `IBugTarget`."""
>>> return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>>>
>>> + def getBranchTips(self, since=None):
>>> + """See `IDistribution`."""
>>> + query = """
>>> + SELECT unique_name, last_scanned_id,
>>> + SeriesSourcePackageBranch.distroseries FROM Branch
>>> + JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
>>> + JOIN Distribution ON DistroSeries.distribution = Distribution.id
>>> + JOIN SeriesSourcePackageBranch ON
>>> + Branch.id = SeriesSourcePackageBranch.branch
>>> + WHERE Distribution.name = %s""" % sqlvalues(self.name)
>>
>> Not all branches will be official, so you want to use a LEFT OUTER JOIN on
>> both SeriesSourcePackageBranch and DistroSeries.
>
> I fixed that and added a test as mentioned below.
>
>> Distro series id is pretty meangingless, we should return their name.
>> DistroSeries.name instead of SeriesSourcePackageBranch.distroseries
>
> Done. I had to tweak the query more than just substituting
> DistroSeries.name but not too much.

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?)

> + 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

The SPBDDS left outer join seems right.

>
>> 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.

>
> 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.

>
>>> === 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.

> + 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 :-)

So with the above comments, all good to go.

  review approve

--
Francis J. Lacoste
<email address hidden>