Launchpad itself

Merge lp:~thumper/launchpad/deleting-individual-branch-revisions into lp:launchpad

  1. deleting-individual-branch-revisions
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Paul Hummer
Approved revision: no longer in the source branch.
Merged at revision: 11534
Proposed branch: lp:~thumper/launchpad/deleting-individual-branch-revisions
Merge into: lp:launchpad
Diff against target: 443 lines (+109/-88)
10 files modified
lib/canonical/config/schema-lazr.conf (+1/-0)
lib/lp/code/configure.zcml (+0/-11)
lib/lp/code/doc/revision.txt (+5/-18)
lib/lp/code/interfaces/branch.py (+6/-0)
lib/lp/code/interfaces/branchrevision.py (+3/-8)
lib/lp/code/model/branch.py (+13/-9)
lib/lp/code/model/branchrevision.py (+4/-18)
lib/lp/code/model/tests/test_branch.py (+56/-2)
lib/lp/codehosting/scanner/bzrsync.py (+20/-17)
lib/lp/codehosting/scanner/tests/test_bzrsync.py (+1/-5)
To merge this branch: bzr merge lp:~thumper/launchpad/deleting-individual-branch-revisions
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+34943@code.launchpad.net

Commit message

Stop deleting branch revision objects by their ids.

Description of the change

This is a pre-branch branch.

The work coming is to remove the BranchRevision.id column. In order to do this we need to update the code that is currently using it.

The only place it is really used is in the branch scanner where it is used to remove BranchRevision rows for revisions that are no longer in the ancestry of a branch due to `push --overwrite`.

The way to delete branch revision objects has been moved onto the branch object itself, and it now handles deleting multiple branch revisions at once.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Diff is a little extreme as I thought that db-stable had been merged back into devel already.

Revision history for this message
Paul Hummer (rockstar) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/config/schema-lazr.conf'
2--- lib/canonical/config/schema-lazr.conf 2010-08-30 00:51:55 +0000
3+++ lib/canonical/config/schema-lazr.conf 2010-09-09 20:24:49 +0000
4@@ -41,6 +41,7 @@
5
6
7 [branchscanner]
8+branch_revision_delete_count: 100
9 # The database user which will be used by this process.
10 # datatype: string
11 dbuser: branchscanner
12
13=== modified file 'lib/lp/code/configure.zcml'
14--- lib/lp/code/configure.zcml 2010-09-01 20:59:44 +0000
15+++ lib/lp/code/configure.zcml 2010-09-09 20:24:49 +0000
16@@ -642,17 +642,6 @@
17 <allow interface="lp.code.interfaces.branchrevision.IBranchRevision"/>
18 </class>
19
20- <!-- BranchRevisionSet -->
21-
22- <class class="lp.code.model.branchrevision.BranchRevisionSet">
23- <allow interface="lp.code.interfaces.branchrevision.IBranchRevisionSet"/>
24- </class>
25- <securedutility
26- class="lp.code.model.branchrevision.BranchRevisionSet"
27- provides="lp.code.interfaces.branchrevision.IBranchRevisionSet">
28- <allow interface="lp.code.interfaces.branchrevision.IBranchRevisionSet"/>
29- </securedutility>
30-
31 <!-- CodeReviewComment -->
32
33 <class class="lp.code.model.codereviewcomment.CodeReviewComment">
34
35=== modified file 'lib/lp/code/doc/revision.txt'
36--- lib/lp/code/doc/revision.txt 2010-07-14 14:50:58 +0000
37+++ lib/lp/code/doc/revision.txt 2010-09-09 20:24:49 +0000
38@@ -130,7 +130,7 @@
39 In particular, IBranch.getScannerData efficiently retrieves the BranchRevision
40 data needed by the branch-scanner script.
41
42- >>> ancestry, history, mapping = branch.getScannerData()
43+ >>> ancestry, history = branch.getScannerData()
44
45 The first return value is a set of revision_id strings for the full ancestry
46 of the branch.
47@@ -158,19 +158,6 @@
48 foo@localhost-20051031170239-5fce7d6bd3f01efc
49 foo@localhost-20051031170357-1301ad6d387feb23
50
51-The third return value is a mapping from revision_id strings to database ids
52-integers of the corresponding BranchRevision rows for this branch.
53-
54- >>> for revision_id, db_id in sorted(mapping.iteritems()):
55- ... print revision_id, db_id
56- foo@localhost-20051031165758-48acedf2b6a2e898 12
57- foo@localhost-20051031170008-098959758bf79803 13
58- foo@localhost-20051031170239-5fce7d6bd3f01efc 14
59- foo@localhost-20051031170357-1301ad6d387feb23 15
60- test@canonical.com-20051031165248-6f1bb97973c2b4f4 10
61- test@canonical.com-20051031165338-5f2f3d6b10bb3bf0 11
62- test@canonical.com-20051031165532-3113df343e494daa 18
63- test@canonical.com-20051031165901-43b9644ec2eacc4e 19
64
65 === Deleting BranchRevisions ===
66
67@@ -180,7 +167,7 @@
68 of the branch, then some of BranchRevision records will need to be
69 removed.
70
71-BranchRevision records are deleted using the `BranchRevisionSet.delete`
72+BranchRevision records are deleted using the `Branch.removeBranchRevisions`
73 method.
74
75
76@@ -197,14 +184,14 @@
77 6
78 >>> revno_6.branch == branch
79 True
80- >>> print revno_6.revision.revision_id
81+ >>> rev_id = revno_6.revision.revision_id
82+ >>> print rev_id
83 foo@localhost-20051031170357-1301ad6d387feb23
84
85 We remove the last revision from the branch. This is similar to what
86 "bzr uncommit" does.
87
88- >>> from lp.code.interfaces.branchrevision import IBranchRevisionSet
89- >>> getUtility(IBranchRevisionSet).delete(revno_6.id)
90+ >>> branch.removeBranchRevisions(rev_id)
91
92 Afterwards, the last commit on the branch has revision number 5.
93
94
95=== modified file 'lib/lp/code/interfaces/branch.py'
96--- lib/lp/code/interfaces/branch.py 2010-09-03 11:54:23 +0000
97+++ lib/lp/code/interfaces/branch.py 2010-09-09 20:24:49 +0000
98@@ -803,6 +803,12 @@
99 def createBranchRevision(sequence, revision):
100 """Create a new `BranchRevision` for this branch."""
101
102+ def removeBranchRevisions(revision_ids):
103+ """Remove the specified revision_ids from this Branch's revisions.
104+
105+ :param revision_ids: Either a single revision_id or an iterable.
106+ """
107+
108 def createBranchRevisionFromIDs(revision_id_sequence_pairs):
109 """Create a batch of BranchRevision objects.
110
111
112=== modified file 'lib/lp/code/interfaces/branchrevision.py'
113--- lib/lp/code/interfaces/branchrevision.py 2010-08-20 20:31:18 +0000
114+++ lib/lp/code/interfaces/branchrevision.py 2010-09-09 20:24:49 +0000
115@@ -6,7 +6,9 @@
116 """BranchRevision interfaces."""
117
118 __metaclass__ = type
119-__all__ = ['IBranchRevision', 'IBranchRevisionSet']
120+__all__ = [
121+ 'IBranchRevision',
122+ ]
123
124 from zope.interface import (
125 Attribute,
126@@ -32,10 +34,3 @@
127 " None for merged revisions which are not part of the history."))
128 branch = Attribute("The branch this revision is included in.")
129 revision = Attribute("A revision that is included in the branch.")
130-
131-
132-class IBranchRevisionSet(Interface):
133- """The set of all branch-revision associations."""
134-
135- def delete(branch_revision_id):
136- """Delete the BranchRevision."""
137
138=== modified file 'lib/lp/code/model/branch.py'
139--- lib/lp/code/model/branch.py 2010-09-03 11:54:23 +0000
140+++ lib/lp/code/model/branch.py 2010-09-09 20:24:49 +0000
141@@ -26,7 +26,6 @@
142 And,
143 Count,
144 Desc,
145- Max,
146 NamedFunc,
147 Not,
148 Or,
149@@ -58,12 +57,8 @@
150 ILaunchpadCelebrities,
151 IPrivacy,
152 )
153+from canonical.launchpad.interfaces.lpstorm import IMasterStore
154 from canonical.launchpad.webapp import urlappend
155-from canonical.launchpad.webapp.interfaces import (
156- IStoreSelector,
157- MAIN_STORE,
158- SLAVE_FLAVOR,
159- )
160 from lp.app.errors import UserCannotUnsubscribePerson
161 from lp.buildmaster.model.buildqueue import BuildQueue
162 from lp.code.bzr import (
163@@ -804,6 +799,17 @@
164 BranchRevision.branch == self,
165 query).one()
166
167+ def removeBranchRevisions(self, revision_ids):
168+ """See `IBranch`."""
169+ if isinstance(revision_ids, basestring):
170+ revision_ids = [revision_ids]
171+ IMasterStore(BranchRevision).find(
172+ BranchRevision,
173+ BranchRevision.branch == self,
174+ BranchRevision.revision_id.is_in(
175+ Select(Revision.id,
176+ Revision.revision_id.is_in(revision_ids)))).remove()
177+
178 def createBranchRevision(self, sequence, revision):
179 """See `IBranch`."""
180 branch_revision = BranchRevision(
181@@ -921,13 +927,11 @@
182 rows = rows.order_by(BranchRevision.sequence)
183 ancestry = set()
184 history = []
185- branch_revision_map = {}
186 for branch_revision_id, sequence, revision_id in rows:
187 ancestry.add(revision_id)
188- branch_revision_map[revision_id] = branch_revision_id
189 if sequence is not None:
190 history.append(revision_id)
191- return ancestry, history, branch_revision_map
192+ return ancestry, history
193
194 def getPullURL(self):
195 """See `IBranch`."""
196
197=== modified file 'lib/lp/code/model/branchrevision.py'
198--- lib/lp/code/model/branchrevision.py 2010-08-20 20:31:18 +0000
199+++ lib/lp/code/model/branchrevision.py 2010-09-09 20:24:49 +0000
200@@ -4,7 +4,9 @@
201 # pylint: disable-msg=E0611,W0212
202
203 __metaclass__ = type
204-__all__ = ['BranchRevision', 'BranchRevisionSet']
205+__all__ = [
206+ 'BranchRevision',
207+ ]
208
209 from storm.locals import (
210 Int,
211@@ -13,11 +15,7 @@
212 )
213 from zope.interface import implements
214
215-from canonical.launchpad.interfaces.lpstorm import IMasterStore
216-from lp.code.interfaces.branchrevision import (
217- IBranchRevision,
218- IBranchRevisionSet,
219- )
220+from lp.code.interfaces.branchrevision import IBranchRevision
221
222
223 class BranchRevision(Storm):
224@@ -40,15 +38,3 @@
225 self.branch = branch
226 self.revision = revision
227 self.sequence = sequence
228-
229-
230-class BranchRevisionSet:
231- """See `IBranchRevisionSet`."""
232-
233- implements(IBranchRevisionSet)
234-
235- def delete(self, branch_revision_id):
236- """See `IBranchRevisionSet`."""
237- match = IMasterStore(BranchRevision).find(
238- BranchRevision, BranchRevision.id == branch_revision_id)
239- match.remove()
240
241=== modified file 'lib/lp/code/model/tests/test_branch.py'
242--- lib/lp/code/model/tests/test_branch.py 2010-09-03 00:25:53 +0000
243+++ lib/lp/code/model/tests/test_branch.py 2010-09-09 20:24:49 +0000
244@@ -29,6 +29,7 @@
245 from canonical.database.constants import UTC_NOW
246 from canonical.launchpad import _
247 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
248+from canonical.launchpad.interfaces.lpstorm import IStore
249 from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
250 from canonical.testing import (
251 DatabaseFunctionalLayer,
252@@ -100,16 +101,16 @@
253 ReclaimBranchSpaceJob,
254 )
255 from lp.code.model.branchmergeproposal import BranchMergeProposal
256+from lp.code.model.branchrevision import BranchRevision
257 from lp.code.model.codeimport import (
258 CodeImport,
259 CodeImportSet,
260 )
261 from lp.code.model.codereviewcomment import CodeReviewComment
262+from lp.code.model.revision import Revision
263 from lp.code.tests.helpers import add_revision_to_branch
264 from lp.codehosting.bzrutils import UnsafeUrlSeen
265-from lp.registry.interfaces.person import IPersonSet
266 from lp.registry.interfaces.pocket import PackagePublishingPocket
267-from lp.registry.model.product import ProductSet
268 from lp.registry.model.sourcepackage import SourcePackage
269 from lp.services.osutils import override_environ
270 from lp.testing import (
271@@ -252,6 +253,59 @@
272 branch.repository_format))
273
274
275+class TestBranchRevisionMethods(TestCaseWithFactory):
276+ """Test the branch methods for adding and removing branch revisions."""
277+
278+ layer = DatabaseFunctionalLayer
279+
280+ def _getBranchRevision(self, branch, rev_id):
281+ """Get the branch revision for the specified branch and rev_id."""
282+ resultset = IStore(BranchRevision).find(
283+ BranchRevision,
284+ BranchRevision.branch == branch,
285+ BranchRevision.revision == Revision.id,
286+ Revision.revision_id == rev_id)
287+ return resultset.one()
288+
289+ def test_createBranchRevision(self):
290+ # createBranchRevision adds the link for the revision to the branch.
291+ branch = self.factory.makeBranch()
292+ rev = self.factory.makeRevision()
293+ # Nothing there to start with.
294+ self.assertIs(None, self._getBranchRevision(branch, rev.revision_id))
295+ branch.createBranchRevision(1, rev)
296+ # Now there is one.
297+ br = self._getBranchRevision(branch, rev.revision_id)
298+ self.assertEqual(branch, br.branch)
299+ self.assertEqual(rev, br.revision)
300+
301+ def test_removeBranchRevisions(self):
302+ # removeBranchRevisions can remove a single linked revision.
303+ branch = self.factory.makeBranch()
304+ rev = self.factory.makeRevision()
305+ branch.createBranchRevision(1, rev)
306+ # Now remove the branch revision.
307+ branch.removeBranchRevisions(rev.revision_id)
308+ # Revision not there now.
309+ self.assertIs(None, self._getBranchRevision(branch, rev.revision_id))
310+
311+ def test_removeBranchRevisions_multiple(self):
312+ # removeBranchRevisions can remove multiple revision links at once.
313+ branch = self.factory.makeBranch()
314+ rev1 = self.factory.makeRevision()
315+ rev2 = self.factory.makeRevision()
316+ rev3 = self.factory.makeRevision()
317+ branch.createBranchRevision(1, rev1)
318+ branch.createBranchRevision(2, rev2)
319+ branch.createBranchRevision(3, rev3)
320+ # Now remove the branch revision.
321+ branch.removeBranchRevisions(
322+ [rev1.revision_id, rev2.revision_id, rev3.revision_id])
323+ # No mainline revisions there now.
324+ # The revision_history attribute is tested above.
325+ self.assertEqual([], list(branch.revision_history))
326+
327+
328 class TestBranchGetRevision(TestCaseWithFactory):
329 """Make sure that `Branch.getBranchRevision` works as expected."""
330
331
332=== modified file 'lib/lp/codehosting/scanner/bzrsync.py'
333--- lib/lp/codehosting/scanner/bzrsync.py 2010-08-20 20:31:18 +0000
334+++ lib/lp/codehosting/scanner/bzrsync.py 2010-09-09 20:24:49 +0000
335@@ -21,8 +21,9 @@
336 from zope.component import getUtility
337 from zope.event import notify
338
339+from canonical.config import config
340+
341 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
342-from lp.code.interfaces.branchrevision import IBranchRevisionSet
343 from lp.code.interfaces.revision import IRevisionSet
344 from lp.codehosting import iter_list_chunks
345 from lp.codehosting.scanner import events
346@@ -82,13 +83,11 @@
347 # written to by the branch-scanner, so they are not subject to
348 # write-lock contention. Update them all in a single transaction to
349 # improve the performance and allow garbage collection in the future.
350- db_ancestry, db_history, db_branch_revision_map = (
351- self.retrieveDatabaseAncestry())
352+ db_ancestry, db_history = self.retrieveDatabaseAncestry()
353
354 (added_ancestry, branchrevisions_to_delete,
355 revids_to_insert) = self.planDatabaseChanges(
356- bzr_branch, bzr_ancestry, bzr_history, db_ancestry, db_history,
357- db_branch_revision_map)
358+ bzr_branch, bzr_ancestry, bzr_history, db_ancestry, db_history)
359 added_ancestry.difference_update(
360 getUtility(IRevisionSet).onlyPresent(added_ancestry))
361 self.logger.info("Adding %s new revisions.", len(added_ancestry))
362@@ -129,9 +128,8 @@
363 def retrieveDatabaseAncestry(self):
364 """Efficiently retrieve ancestry from the database."""
365 self.logger.info("Retrieving ancestry from database.")
366- db_ancestry, db_history, db_branch_revision_map = (
367- self.db_branch.getScannerData())
368- return db_ancestry, db_history, db_branch_revision_map
369+ db_ancestry, db_history = self.db_branch.getScannerData()
370+ return db_ancestry, db_history
371
372 def retrieveBranchDetails(self, bzr_branch):
373 """Retrieve ancestry from the the bzr branch on disk."""
374@@ -147,7 +145,7 @@
375 return bzr_ancestry, bzr_history
376
377 def planDatabaseChanges(self, bzr_branch, bzr_ancestry, bzr_history,
378- db_ancestry, db_history, db_branch_revision_map):
379+ db_ancestry, db_history):
380 """Plan database changes to synchronize with bzrlib data.
381
382 Use the data retrieved by `retrieveDatabaseAncestry` and
383@@ -190,9 +188,8 @@
384
385 # We must delete BranchRevision rows for all revisions which where
386 # removed from the ancestry or whose sequence value has changed.
387- branchrevisions_to_delete = set(
388- db_branch_revision_map[revid]
389- for revid in removed_merged.union(removed_history))
390+ branchrevisions_to_delete = list(
391+ removed_merged.union(removed_history))
392
393 # We must insert BranchRevision rows for all revisions which were
394 # added to the ancestry or whose sequence value has changed.
395@@ -244,13 +241,19 @@
396 for revision_id in revision_subset.difference(set(bzr_history)):
397 yield revision_id, None
398
399- def deleteBranchRevisions(self, branchrevisions_to_delete):
400+ def deleteBranchRevisions(self, revision_ids_to_delete):
401 """Delete a batch of BranchRevision rows."""
402 self.logger.info("Deleting %d branchrevision records.",
403- len(branchrevisions_to_delete))
404- branch_revision_set = getUtility(IBranchRevisionSet)
405- for branchrevision in sorted(branchrevisions_to_delete):
406- branch_revision_set.delete(branchrevision)
407+ len(revision_ids_to_delete))
408+ # Use a config value to work out how many to delete at a time.
409+ # Deleting more than one at a time is significantly more efficient
410+ # than doing one at a time, but the actual optimal count is a bit up
411+ # in the air.
412+ batch_size = config.branchscanner.branch_revision_delete_count
413+ while revision_ids_to_delete:
414+ batch = revision_ids_to_delete[:batch_size]
415+ revision_ids_to_delete[:batch_size] = []
416+ self.db_branch.removeBranchRevisions(batch)
417
418 def insertBranchRevisions(self, bzr_branch, revids_to_insert):
419 """Insert a batch of BranchRevision rows."""
420
421=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
422--- lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-08-20 20:31:18 +0000
423+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-09-09 20:24:49 +0000
424@@ -490,18 +490,14 @@
425 expected_history = [branch_revision.revision.revision_id
426 for branch_revision in sampledata
427 if branch_revision.sequence is not None]
428- expected_mapping = dict(
429- (branch_revision.revision.revision_id, branch_revision.id)
430- for branch_revision in sampledata)
431
432 self.create_branch_and_tree(db_branch=branch)
433
434 bzrsync = self.makeBzrSync(branch)
435- db_ancestry, db_history, db_branch_revision_map = (
436+ db_ancestry, db_history = (
437 bzrsync.retrieveDatabaseAncestry())
438 self.assertEqual(expected_ancestry, set(db_ancestry))
439 self.assertEqual(expected_history, list(db_history))
440- self.assertEqual(expected_mapping, db_branch_revision_map)
441
442
443 class TestBzrSyncOneRevision(BzrSyncTestCase):