Merge lp:~jtv/launchpad/bug-733245 into lp:launchpad

Proposed by Jeroen T. Vermeulen on 2011-03-14
Status: Merged
Approved by: Jeroen T. Vermeulen on 2011-03-15
Approved revision: no longer in the source branch.
Merged at revision: 12599
Proposed branch: lp:~jtv/launchpad/bug-733245
Merge into: lp:launchpad
Diff against target: 288 lines (+175/-8)
3 files modified
lib/lp/registry/model/distroseriesdifference.py (+1/-2)
lib/lp/registry/scripts/populate_distroseriesdiff.py (+88/-4)
lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py (+86/-2)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-733245
Reviewer Review Type Date Requested Status
Benji York (community) code 2011-03-14 Approve on 2011-03-14
Review via email: mp+53230@code.launchpad.net

Commit message

[r=benji][bug=733245] Initialize DSD.base_version in populating script.

Description of the change

= Summary =

The DistroSeriesDifference population script creates DSDs, but leaves base_version uninitialized. That part needs to be done from python.

== Proposed fix ==

Fire off a DBLoopTuner to fix up any DSDs for a freshly-populated DistroSeries. The work is likely to take long enough to warrant a DBLoopTuner; that way we know it won't cause undue stress on the backend.

If we enable the feature before the fixup is done, we'll start out with DSDs where it's impossible to request a diff. But that may be better than leaving the feature disabled until all the work is finished.

== Pre-implementation notes ==

This work is subject to more or less continuous discussion. In particular in this case, I discussed with StevenK.

== Implementation details ==

A new option --ignore-base-version lets the script ignore base_version. If desired, we can run the script with this option to create all required DSDs very quickly, and then run it again without the option to fix up the base_versions.

Conversely, there is no mode to skip creation of DSDs; that part is fast and unintrusive enough to run multiple times — especially since it does not update or re-create existing DSDs. Just detecting which DSDs there should be takes a few seconds at worst.

== Tests ==

{{{
./bin/test -vvc lp.registry.scripts.tests.test_populate_distroseriesdiff
}}}

== Demo and Q/A ==

We'll run this on a full derived distroseries and check that some of the base_versions get updated. It won't be all, since it's not always possible to determine a usable base_version.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/scripts/populate_distroseriesdiff.py
  lib/lp/registry/model/distroseriesdifference.py
  lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py

To post a comment you must log in.
Benji York (benji) wrote :

Looks good.

Line 206 of lib/lp/registry/scripts/populate_distroseriesdiff.py (line
49 of the diff): I think you want to wrap temp_table in a call to
quote_identifier.

The use of the expression "3 - 1" in test_cutChunk_one_cuts_exactly_one
is creative and increases the clarity of the test.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
2--- lib/lp/registry/model/distroseriesdifference.py 2011-03-10 04:00:08 +0000
3+++ lib/lp/registry/model/distroseriesdifference.py 2011-03-14 12:23:49 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2010 Canonical Ltd. This software is licensed under the
6+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """Database classes for a difference between two distribution series."""
10@@ -29,7 +29,6 @@
11 IMasterStore,
12 IStore,
13 )
14-from lp.archivepublisher.debversion import Version
15 from lp.registry.enum import (
16 DistroSeriesDifferenceStatus,
17 DistroSeriesDifferenceType,
18
19=== modified file 'lib/lp/registry/scripts/populate_distroseriesdiff.py'
20--- lib/lp/registry/scripts/populate_distroseriesdiff.py 2011-03-04 05:50:01 +0000
21+++ lib/lp/registry/scripts/populate_distroseriesdiff.py 2011-03-14 12:23:49 +0000
22@@ -33,9 +33,11 @@
23 DistroSeriesDifferenceStatus,
24 DistroSeriesDifferenceType,
25 )
26+from canonical.launchpad.utilities.looptuner import TunableLoop
27 from lp.registry.interfaces.distribution import IDistributionSet
28 from lp.registry.interfaces.pocket import PackagePublishingPocket
29 from lp.registry.model.distroseries import DistroSeries
30+from lp.registry.model.distroseriesdifference import DistroSeriesDifference
31 from lp.services.scripts.base import LaunchpadScript
32 from lp.soyuz.interfaces.publishing import active_publishing_status
33
34@@ -188,7 +190,7 @@
35 store.execute("DROP TABLE IF EXISTS %s" % quote_identifier(table))
36
37
38-def populate_distroseriesdiff(derived_distroseries):
39+def populate_distroseriesdiff(logger, derived_distroseries):
40 """Compare `derived_distroseries` to parent, and register differences.
41
42 The differences are registered by creating `DistroSeriesDifference`
43@@ -201,6 +203,9 @@
44 store.execute("CREATE TEMP TABLE %s AS %s" % (
45 quote_identifier(temp_table),
46 compose_sql_find_differences(derived_distroseries)))
47+ logger.info(
48+ "Found %d potential difference(s).",
49+ store.execute("SELECT count(*) FROM %s" % temp_table).get_one()[0])
50 store.execute(
51 compose_sql_populate_distroseriesdiff(
52 derived_distroseries, temp_table))
53@@ -221,6 +226,59 @@
54 (DistroSeries.parent_seriesID, DistroSeries.id))
55
56
57+class BaseVersionFixer(TunableLoop):
58+ """Fix up `DistroSeriesDifference.base_version` in the database.
59+
60+ The code that creates `DistroSeriesDifference`s does not set the
61+ `base_version`. In cases where there may actually be a real base
62+ version, this needs to be fixed up.
63+
64+ Since this is likely to be much, much slower than the rest of the
65+ work of creating and initializing `DistroSeriesDifference`s, it is
66+ done in a `DBLoopTuner`.
67+ """
68+
69+ def __init__(self, log, store, commit, ids):
70+ """See `TunableLoop`.
71+
72+ :param log: A logger.
73+ :param store: Database store to work on.
74+ :param commit: A commit function to call after each batch.
75+ :param ids: Sequence of `DistroSeriesDifference` ids to fix.
76+ """
77+ super(BaseVersionFixer, self).__init__(log)
78+ self.minimum_chunk_size = 2
79+ self.maximum_chunk_size = 1000
80+ self.store = store
81+ self.commit = commit
82+ self.ids = sorted(ids, reverse=True)
83+
84+ def isDone(self):
85+ """See `ITunableLoop`."""
86+ return len(self.ids) == 0
87+
88+ def _cutChunk(self, chunk_size):
89+ """Cut a chunk of up to `chunk_size` items from the remaining work.
90+
91+ Removes the items to be processed in this chunk from the list of
92+ remaining work, and returns those.
93+ """
94+ todo = self.ids[-chunk_size:]
95+ self.ids = self.ids[:-chunk_size]
96+ return todo
97+
98+ def _getBatch(self, ids):
99+ """Retrieve a batch of `DistroSeriesDifference`s with given ids."""
100+ return self.store.find(
101+ DistroSeriesDifference, DistroSeriesDifference.id.is_in(ids))
102+
103+ def __call__(self, chunk_size):
104+ """See `ITunableLoop`."""
105+ for dsd in self._getBatch(self._cutChunk(int(chunk_size))):
106+ dsd._updateBaseVersion()
107+ self.commit()
108+
109+
110 class PopulateDistroSeriesDiff(LaunchpadScript):
111 """Populate `DistroSeriesDifference` for pre-existing differences."""
112
113@@ -264,7 +322,10 @@
114 def processDistroSeries(self, distroseries):
115 """Generate `DistroSeriesDifference`s for `distroseries`."""
116 self.logger.info("Looking for differences in %s.", distroseries)
117- populate_distroseriesdiff(distroseries)
118+ populate_distroseriesdiff(self.logger, distroseries)
119+ self.commit()
120+ self.logger.info("Updating base_versions.")
121+ self.fixBaseVersions(distroseries)
122 self.commit()
123 self.logger.info("Done with %s.", distroseries)
124
125@@ -288,10 +349,10 @@
126 specified_series = (self.options.series is not None)
127 if specified_distro != specified_series:
128 raise OptionValueError(
129- "Specify neither a distribution or a series, or both.")
130+ "Specify both a distribution and a series, or use --all.")
131 if specified_distro == self.options.all:
132 raise OptionValueError(
133- "Either specify a distribution series, or use --all.")
134+ "Either specify a distribution and series, or use --all.")
135
136 def main(self):
137 """Do the script's work."""
138@@ -307,3 +368,26 @@
139
140 for series in self.getDistroSeries():
141 self.processDistroSeries(series)
142+
143+ def fixBaseVersions(self, distroseries):
144+ """Fix up `DistroSeriesDifference.base_version` where appropriate.
145+
146+ The `DistroSeriesDifference` records we create don't have their
147+ `base_version` fields set yet. This is a shame because it's the
148+ only thing we need to figure out python-side.
149+
150+ Only instances where the source package is published in both the
151+ parent series and the derived series need to have this done.
152+ """
153+ self.logger.info(
154+ "Fixing up base_versions for %s.", distroseries.title)
155+ store = IStore(distroseries)
156+ dsd_ids = store.find(
157+ DistroSeriesDifference.id,
158+ DistroSeriesDifference.derived_series == distroseries,
159+ DistroSeriesDifference.status ==
160+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
161+ DistroSeriesDifference.difference_type ==
162+ DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
163+ DistroSeriesDifference.base_version == None)
164+ BaseVersionFixer(self.logger, store, self.commit, dsd_ids).run()
165
166=== modified file 'lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py'
167--- lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-03-11 00:29:38 +0000
168+++ lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-03-14 12:23:49 +0000
169@@ -24,6 +24,7 @@
170 from lp.registry.interfaces.pocket import PackagePublishingPocket
171 from lp.registry.model.distroseriesdifference import DistroSeriesDifference
172 from lp.registry.scripts.populate_distroseriesdiff import (
173+ BaseVersionFixer,
174 compose_sql_difference_type,
175 compose_sql_find_latest_source_package_releases,
176 compose_sql_find_differences,
177@@ -42,7 +43,10 @@
178 )
179 from lp.soyuz.model.archive import Archive
180 from lp.soyuz.enums import ArchivePurpose
181-from lp.testing import TestCaseWithFactory
182+from lp.testing import (
183+ TestCase,
184+ TestCaseWithFactory,
185+ )
186 from lp.testing.fakemethod import FakeMethod
187
188
189@@ -409,7 +413,7 @@
190 def test_creates_distroseriesdifference(self):
191 distroseries = self.makeDerivedDistroSeries()
192 spph = self.makeSPPH(distroseries=distroseries)
193- populate_distroseriesdiff(distroseries)
194+ populate_distroseriesdiff(DevNullLogger(), distroseries)
195 store = Store.of(distroseries)
196 dsd = self.getDistroSeriesDiff(distroseries).one()
197 spr = spph.sourcepackagerelease
198@@ -445,6 +449,50 @@
199 self.assertEqual(existing_versions['derived'], dsd.source_version)
200
201
202+class FakeDSD:
203+ _updateBaseVersion = FakeMethod()
204+
205+
206+class TestBaseVersionFixer(TestCase):
207+ """Test the poignant parts of `BaseVersionFixer`."""
208+
209+ def makeFixer(self, ids):
210+ fixer = BaseVersionFixer(DevNullLogger(), None, FakeMethod(), ids)
211+ fixer._getBatch = FakeMethod()
212+ return fixer
213+
214+ def test_isDone_is_done_when_ids_is_empty(self):
215+ self.assertTrue(self.makeFixer([]).isDone())
216+
217+ def test_isDone_is_not_done_until_ids_is_empty(self):
218+ self.assertFalse(self.makeFixer([1]).isDone())
219+
220+ def test_cutChunk_one_cuts_exactly_one(self):
221+ fixer = self.makeFixer(range(3))
222+ chunk = fixer._cutChunk(1)
223+ self.assertEqual([0], chunk)
224+ self.assertEqual(3 - 1, len(fixer.ids))
225+
226+ def test_cutChunk_over_remaining_size_completes_loop(self):
227+ fixer = self.makeFixer(range(3))
228+ chunk = fixer._cutChunk(100)
229+ self.assertContentEqual(range(3), chunk)
230+ self.assertEqual([], fixer.ids)
231+
232+ def test_updatesBaseVersion(self):
233+ fake_dsd = FakeDSD()
234+ fixer = self.makeFixer([fake_dsd])
235+ fixer._getBatch.result = fixer.ids
236+ fixer(1)
237+ self.assertNotEqual(0, fake_dsd._updateBaseVersion.call_count)
238+
239+ def test_loop_commits(self):
240+ fixer = self.makeFixer([FakeDSD()])
241+ fixer._getBatch = FakeMethod(result=fixer.ids)
242+ fixer(1)
243+ self.assertNotEqual(0, fixer.commit.call_count)
244+
245+
246 class TestPopulateDistroSeriesDiffScript(TestCaseWithFactory, FactoryHelper):
247 """Test the `populate-distroseriesdiff` script."""
248
249@@ -522,3 +570,39 @@
250 expected_series_name = "%s %s" % (
251 spph.distroseries.distribution.name, spph.distroseries.name)
252 self.assertIn(expected_series_name, script.logger.getLogBuffer())
253+
254+ def test_calls_fixBaseVersions(self):
255+ distroseries = self.makeDerivedDistroSeries()
256+ self.makeSPPH(distroseries=distroseries)
257+ script = self.makeScript([
258+ '--distribution', distroseries.distribution.name,
259+ '--series', distroseries.name,
260+ ])
261+ script.fixBaseVersions = FakeMethod()
262+ script.main()
263+ self.assertEqual(
264+ [((distroseries,), {})], script.fixBaseVersions.calls)
265+
266+ def test_fixes_base_versions(self):
267+ distroseries = self.makeDerivedDistroSeries()
268+ package = self.factory.makeSourcePackageName()
269+ derived_spr = self.factory.makeSourcePackageRelease(
270+ distroseries=distroseries, sourcepackagename=package)
271+ self.makeSPPH(
272+ distroseries=distroseries,
273+ sourcepackagerelease=derived_spr)
274+ parent_spr = self.factory.makeSourcePackageRelease(
275+ distroseries=distroseries.parent_series,
276+ sourcepackagename=package)
277+ self.makeSPPH(
278+ distroseries=distroseries.parent_series,
279+ sourcepackagerelease=parent_spr)
280+ script = self.makeScript([
281+ '--distribution', distroseries.distribution.name,
282+ '--series', distroseries.name,
283+ ])
284+ script.main()
285+ dsd = self.getDistroSeriesDiff(distroseries)[0]
286+ dsd._updateBaseVersion = FakeMethod()
287+ script.fixBaseVersions(distroseries)
288+ self.assertEqual(1, dsd._updateBaseVersion.call_count)