Merge lp:~mwhudson/launchpad/code-import-optimizations into lp:launchpad
- code-import-optimizations
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Tim Penhey |
Approved revision: | 10680 |
Merged at revision: | not available |
Proposed branch: | lp:~mwhudson/launchpad/code-import-optimizations |
Merge into: | lp:launchpad |
Diff against target: |
555 lines (+176/-126) 2 files modified
lib/lp/codehosting/codeimport/tests/test_worker.py (+87/-52) lib/lp/codehosting/codeimport/worker.py (+89/-74) |
To merge this branch: | bzr merge lp:~mwhudson/launchpad/code-import-optimizations |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Penhey (community) | Approve | ||
Review via email: mp+23206@code.launchpad.net |
Commit message
Improve logging during code imports and don't build a working tree if not needed, both of which will help with large imports timing out on startup
Description of the change
Hi,
This branch should fix some very large foreign branch imports timing out in two ways:
1) There was no progress reporting during fetching the branch from the store or building the tree, so I moved the initial fetch into the block that has our custom ui factory installed.
2) Building the working tree was a waste of time anyway for foreign branch imports, so we no longer do that. This required the bulk of the changes from a textual point of view, to talk about branches where previously we talked about trees, but I think the result is better even if it wasn't faster.
I also add a few logging calls as suggested in bug 559808.
Cheers,
mwh
Michael Hudson-Doyle (mwhudson) wrote : | # |
On 12/04/10 14:16, Tim Penhey wrote:
> Review: Approve
> The change looks fine.
Thanks.
> It does get me wondering a bit why we didn't seem to get any logging when it was pushing the branch to the central store before. Thanks for adding the job started and finished logging bits too.
I think we do actually get logging when pushing to the central store,
but the new log lines should let us see what comes from where :-)
> I agree that it makes more sense to use branch rather than working tree in the workers.
Cool.
Cheers,
mwh
Preview Diff
1 | === modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py' | |||
2 | --- lib/lp/codehosting/codeimport/tests/test_worker.py 2010-04-09 04:39:38 +0000 | |||
3 | +++ lib/lp/codehosting/codeimport/tests/test_worker.py 2010-04-12 01:54:17 +0000 | |||
4 | @@ -19,7 +19,7 @@ | |||
5 | 19 | from bzrlib.tests import TestCaseWithTransport | 19 | from bzrlib.tests import TestCaseWithTransport |
6 | 20 | from bzrlib.transport import get_transport | 20 | from bzrlib.transport import get_transport |
7 | 21 | from bzrlib.upgrade import upgrade | 21 | from bzrlib.upgrade import upgrade |
9 | 22 | from bzrlib.urlutils import join as urljoin | 22 | from bzrlib.urlutils import join as urljoin, local_path_from_url |
10 | 23 | 23 | ||
11 | 24 | from CVS import Repository, tree as CVSTree | 24 | from CVS import Repository, tree as CVSTree |
12 | 25 | 25 | ||
13 | @@ -120,22 +120,57 @@ | |||
14 | 120 | 120 | ||
15 | 121 | def test_getNewBranch(self): | 121 | def test_getNewBranch(self): |
16 | 122 | # If there's no Bazaar branch of this id, then pull creates a new | 122 | # If there's no Bazaar branch of this id, then pull creates a new |
18 | 123 | # Bazaar working tree. | 123 | # Bazaar branch. |
19 | 124 | store = self.makeBranchStore() | 124 | store = self.makeBranchStore() |
21 | 125 | bzr_working_tree = store.pull( | 125 | bzr_branch = store.pull( |
22 | 126 | self.arbitrary_branch_id, self.temp_dir, default_format) | 126 | self.arbitrary_branch_id, self.temp_dir, default_format) |
24 | 127 | self.assertEqual([], bzr_working_tree.branch.revision_history()) | 127 | self.assertEqual([], bzr_branch.revision_history()) |
25 | 128 | |||
26 | 129 | def test_getNewBranch_without_tree(self): | ||
27 | 130 | # If pull() with needs_tree=False creates a new branch, it doesn't | ||
28 | 131 | # create a working tree. | ||
29 | 132 | store = self.makeBranchStore() | ||
30 | 133 | bzr_branch = store.pull( | ||
31 | 134 | self.arbitrary_branch_id, self.temp_dir, default_format, False) | ||
32 | 135 | self.assertFalse(bzr_branch.bzrdir.has_workingtree()) | ||
33 | 136 | |||
34 | 137 | def test_getNewBranch_with_tree(self): | ||
35 | 138 | # If pull() with needs_tree=True creates a new branch, it creates a | ||
36 | 139 | # working tree. | ||
37 | 140 | store = self.makeBranchStore() | ||
38 | 141 | bzr_branch = store.pull( | ||
39 | 142 | self.arbitrary_branch_id, self.temp_dir, default_format, True) | ||
40 | 143 | self.assertTrue(bzr_branch.bzrdir.has_workingtree()) | ||
41 | 128 | 144 | ||
42 | 129 | def test_pushBranchThenPull(self): | 145 | def test_pushBranchThenPull(self): |
43 | 130 | # After we've pushed up a branch to the store, we can then pull it | 146 | # After we've pushed up a branch to the store, we can then pull it |
44 | 131 | # from the store. | 147 | # from the store. |
45 | 132 | store = self.makeBranchStore() | 148 | store = self.makeBranchStore() |
46 | 133 | tree = create_branch_with_one_revision('original') | 149 | tree = create_branch_with_one_revision('original') |
49 | 134 | store.push(self.arbitrary_branch_id, tree, default_format) | 150 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
50 | 135 | new_tree = store.pull( | 151 | new_branch = store.pull( |
51 | 136 | self.arbitrary_branch_id, self.temp_dir, default_format) | 152 | self.arbitrary_branch_id, self.temp_dir, default_format) |
52 | 137 | self.assertEqual( | 153 | self.assertEqual( |
54 | 138 | tree.branch.last_revision(), new_tree.branch.last_revision()) | 154 | tree.branch.last_revision(), new_branch.last_revision()) |
55 | 155 | |||
56 | 156 | def test_pull_without_needs_tree_doesnt_create_tree(self): | ||
57 | 157 | # pull with needs_tree=False doesn't spend the time to create a | ||
58 | 158 | # working tree. | ||
59 | 159 | store = self.makeBranchStore() | ||
60 | 160 | tree = create_branch_with_one_revision('original') | ||
61 | 161 | store.push(self.arbitrary_branch_id, tree.branch, default_format) | ||
62 | 162 | new_branch = store.pull( | ||
63 | 163 | self.arbitrary_branch_id, self.temp_dir, default_format, False) | ||
64 | 164 | self.assertFalse(new_branch.bzrdir.has_workingtree()) | ||
65 | 165 | |||
66 | 166 | def test_pull_needs_tree_creates_tree(self): | ||
67 | 167 | # pull with needs_tree=True creates a working tree. | ||
68 | 168 | store = self.makeBranchStore() | ||
69 | 169 | tree = create_branch_with_one_revision('original') | ||
70 | 170 | store.push(self.arbitrary_branch_id, tree.branch, default_format) | ||
71 | 171 | new_branch = store.pull( | ||
72 | 172 | self.arbitrary_branch_id, self.temp_dir, default_format, True) | ||
73 | 173 | self.assertTrue(new_branch.bzrdir.has_workingtree()) | ||
74 | 139 | 174 | ||
75 | 140 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import | 175 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import |
76 | 141 | # branches disabled. Need an orderly upgrade process. | 176 | # branches disabled. Need an orderly upgrade process. |
77 | @@ -188,21 +223,21 @@ | |||
78 | 188 | # store. | 223 | # store. |
79 | 189 | store = self.makeBranchStore() | 224 | store = self.makeBranchStore() |
80 | 190 | tree = create_branch_with_one_revision('original') | 225 | tree = create_branch_with_one_revision('original') |
84 | 191 | store.push(self.arbitrary_branch_id, tree, default_format) | 226 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
85 | 192 | store.push(self.arbitrary_branch_id, tree, default_format) | 227 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
86 | 193 | new_tree = store.pull( | 228 | new_branch = store.pull( |
87 | 194 | self.arbitrary_branch_id, self.temp_dir, default_format) | 229 | self.arbitrary_branch_id, self.temp_dir, default_format) |
88 | 195 | self.assertEqual( | 230 | self.assertEqual( |
90 | 196 | tree.branch.last_revision(), new_tree.branch.last_revision()) | 231 | tree.branch.last_revision(), new_branch.last_revision()) |
91 | 197 | 232 | ||
92 | 198 | def test_push_divergant_branches(self): | 233 | def test_push_divergant_branches(self): |
93 | 199 | # push() uses overwrite=True, so divergent branches (rebased) can be | 234 | # push() uses overwrite=True, so divergent branches (rebased) can be |
94 | 200 | # pushed. | 235 | # pushed. |
95 | 201 | store = self.makeBranchStore() | 236 | store = self.makeBranchStore() |
96 | 202 | tree = create_branch_with_one_revision('original') | 237 | tree = create_branch_with_one_revision('original') |
98 | 203 | store.push(self.arbitrary_branch_id, tree, default_format) | 238 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
99 | 204 | tree = create_branch_with_one_revision('divergant') | 239 | tree = create_branch_with_one_revision('divergant') |
101 | 205 | store.push(self.arbitrary_branch_id, tree, default_format) | 240 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
102 | 206 | 241 | ||
103 | 207 | def fetchBranch(self, from_url, target_path): | 242 | def fetchBranch(self, from_url, target_path): |
104 | 208 | """Pull a branch from `from_url` to `target_path`. | 243 | """Pull a branch from `from_url` to `target_path`. |
105 | @@ -221,7 +256,7 @@ | |||
106 | 221 | # doesn't already exist. | 256 | # doesn't already exist. |
107 | 222 | store = BazaarBranchStore(self.get_transport('doesntexist')) | 257 | store = BazaarBranchStore(self.get_transport('doesntexist')) |
108 | 223 | tree = create_branch_with_one_revision('original') | 258 | tree = create_branch_with_one_revision('original') |
110 | 224 | store.push(self.arbitrary_branch_id, tree, default_format) | 259 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
111 | 225 | self.assertIsDirectory('doesntexist', self.get_transport()) | 260 | self.assertIsDirectory('doesntexist', self.get_transport()) |
112 | 226 | 261 | ||
113 | 227 | def test_storedLocation(self): | 262 | def test_storedLocation(self): |
114 | @@ -229,12 +264,12 @@ | |||
115 | 229 | # the BazaarBranchStore's transport. | 264 | # the BazaarBranchStore's transport. |
116 | 230 | store = self.makeBranchStore() | 265 | store = self.makeBranchStore() |
117 | 231 | tree = create_branch_with_one_revision('original') | 266 | tree = create_branch_with_one_revision('original') |
120 | 232 | store.push(self.arbitrary_branch_id, tree, default_format) | 267 | store.push(self.arbitrary_branch_id, tree.branch, default_format) |
121 | 233 | new_tree = self.fetchBranch( | 268 | new_branch = self.fetchBranch( |
122 | 234 | urljoin(store.transport.base, '%08x' % self.arbitrary_branch_id), | 269 | urljoin(store.transport.base, '%08x' % self.arbitrary_branch_id), |
123 | 235 | 'new_tree') | 270 | 'new_tree') |
124 | 236 | self.assertEqual( | 271 | self.assertEqual( |
126 | 237 | tree.branch.last_revision(), new_tree.branch.last_revision()) | 272 | tree.branch.last_revision(), new_branch.last_revision()) |
127 | 238 | 273 | ||
128 | 239 | def test_sftpPrefix(self): | 274 | def test_sftpPrefix(self): |
129 | 240 | # Since branches are mirrored by importd via sftp, _getMirrorURL must | 275 | # Since branches are mirrored by importd via sftp, _getMirrorURL must |
130 | @@ -270,15 +305,14 @@ | |||
131 | 270 | None, None, [('add', ('', 'root-id', 'directory', ''))]) | 305 | None, None, [('add', ('', 'root-id', 'directory', ''))]) |
132 | 271 | revid1 = builder.build_snapshot(None, [revid], []) | 306 | revid1 = builder.build_snapshot(None, [revid], []) |
133 | 272 | revid2 = builder.build_snapshot(None, [revid], []) | 307 | revid2 = builder.build_snapshot(None, [revid], []) |
134 | 273 | branch = builder.get_branch() | ||
135 | 274 | source_tree = branch.bzrdir.create_workingtree() | ||
136 | 275 | store = self.makeBranchStore() | 308 | store = self.makeBranchStore() |
139 | 276 | store.push(self.arbitrary_branch_id, source_tree, default_format) | 309 | store.push( |
140 | 277 | retrieved_tree = store.pull( | 310 | self.arbitrary_branch_id, builder.get_branch(), default_format) |
141 | 311 | retrieved_branch = store.pull( | ||
142 | 278 | self.arbitrary_branch_id, 'pulled', default_format) | 312 | self.arbitrary_branch_id, 'pulled', default_format) |
143 | 279 | self.assertEqual( | 313 | self.assertEqual( |
144 | 280 | set([revid, revid1, revid2]), | 314 | set([revid, revid1, revid2]), |
146 | 281 | set(retrieved_tree.branch.repository.all_revision_ids())) | 315 | set(retrieved_branch.repository.all_revision_ids())) |
147 | 282 | 316 | ||
148 | 283 | 317 | ||
149 | 284 | class TestImportDataStore(WorkerTest): | 318 | class TestImportDataStore(WorkerTest): |
150 | @@ -532,21 +566,21 @@ | |||
151 | 532 | worker = self.makeImportWorker() | 566 | worker = self.makeImportWorker() |
152 | 533 | self.assertEqual(self.source_details, worker.source_details) | 567 | self.assertEqual(self.source_details, worker.source_details) |
153 | 534 | 568 | ||
156 | 535 | def test_getBazaarWorkingTreeMakesEmptyTree(self): | 569 | def test_getBazaarWorkingBranchMakesEmptyBranch(self): |
157 | 536 | # getBazaarWorkingTree returns a brand-new working tree for an initial | 570 | # getBazaarBranch returns a brand-new working tree for an initial |
158 | 537 | # import. | 571 | # import. |
159 | 538 | worker = self.makeImportWorker() | 572 | worker = self.makeImportWorker() |
162 | 539 | bzr_working_tree = worker.getBazaarWorkingTree() | 573 | bzr_branch = worker.getBazaarBranch() |
163 | 540 | self.assertEqual([], bzr_working_tree.branch.revision_history()) | 574 | self.assertEqual([], bzr_branch.revision_history()) |
164 | 541 | 575 | ||
168 | 542 | def test_bazaarWorkingTreeLocation(self): | 576 | def test_bazaarBranchLocation(self): |
169 | 543 | # getBazaarWorkingTree makes the working tree under the current | 577 | # getBazaarBranch makes the working tree under the current working |
170 | 544 | # working directory. | 578 | # directory. |
171 | 545 | worker = self.makeImportWorker() | 579 | worker = self.makeImportWorker() |
173 | 546 | bzr_working_tree = worker.getBazaarWorkingTree() | 580 | bzr_branch = worker.getBazaarBranch() |
174 | 547 | self.assertIsSameRealPath( | 581 | self.assertIsSameRealPath( |
177 | 548 | os.path.abspath(worker.BZR_WORKING_TREE_PATH), | 582 | os.path.abspath(worker.BZR_BRANCH_PATH), |
178 | 549 | os.path.abspath(bzr_working_tree.basedir)) | 583 | os.path.abspath(local_path_from_url(bzr_branch.base))) |
179 | 550 | 584 | ||
180 | 551 | 585 | ||
181 | 552 | class TestCSCVSWorker(WorkerTest): | 586 | class TestCSCVSWorker(WorkerTest): |
182 | @@ -591,22 +625,22 @@ | |||
183 | 591 | source_details, self.get_transport('import_data'), | 625 | source_details, self.get_transport('import_data'), |
184 | 592 | self.makeBazaarBranchStore(), logging.getLogger("silent")) | 626 | self.makeBazaarBranchStore(), logging.getLogger("silent")) |
185 | 593 | 627 | ||
189 | 594 | def test_pushBazaarWorkingTree_saves_git_cache(self): | 628 | def test_pushBazaarBranch_saves_git_cache(self): |
190 | 595 | # GitImportWorker.pushBazaarWorkingTree saves a tarball of the git | 629 | # GitImportWorker.pushBazaarBranch saves a tarball of the git cache |
191 | 596 | # cache from the tree's repository in the worker's ImportDataStore. | 630 | # from the tree's repository in the worker's ImportDataStore. |
192 | 597 | content = self.factory.getUniqueString() | 631 | content = self.factory.getUniqueString() |
196 | 598 | tree = self.make_branch_and_tree('.') | 632 | branch = self.make_branch('.') |
197 | 599 | tree.branch.repository._transport.mkdir('git') | 633 | branch.repository._transport.mkdir('git') |
198 | 600 | tree.branch.repository._transport.put_bytes('git/cache', content) | 634 | branch.repository._transport.put_bytes('git/cache', content) |
199 | 601 | import_worker = self.makeImportWorker() | 635 | import_worker = self.makeImportWorker() |
201 | 602 | import_worker.pushBazaarWorkingTree(tree) | 636 | import_worker.pushBazaarBranch(branch) |
202 | 603 | import_worker.import_data_store.fetch('git-cache.tar.gz') | 637 | import_worker.import_data_store.fetch('git-cache.tar.gz') |
203 | 604 | extract_tarball('git-cache.tar.gz', '.') | 638 | extract_tarball('git-cache.tar.gz', '.') |
204 | 605 | self.assertEqual(content, open('cache').read()) | 639 | self.assertEqual(content, open('cache').read()) |
205 | 606 | 640 | ||
209 | 607 | def test_getBazaarWorkingTree_fetches_legacy_git_db(self): | 641 | def test_getBazaarBranch_fetches_legacy_git_db(self): |
210 | 608 | # GitImportWorker.getBazaarWorkingTree fetches the legacy git.db file, | 642 | # GitImportWorker.getBazaarBranch fetches the legacy git.db file, if |
211 | 609 | # if present, from the worker's ImportDataStore into the tree's | 643 | # present, from the worker's ImportDataStore into the tree's |
212 | 610 | # repository. | 644 | # repository. |
213 | 611 | import_worker = self.makeImportWorker() | 645 | import_worker = self.makeImportWorker() |
214 | 612 | # Store the git.db file in the store. | 646 | # Store the git.db file in the store. |
215 | @@ -614,15 +648,15 @@ | |||
216 | 614 | open('git.db', 'w').write(content) | 648 | open('git.db', 'w').write(content) |
217 | 615 | import_worker.import_data_store.put('git.db') | 649 | import_worker.import_data_store.put('git.db') |
218 | 616 | # Make sure there's a Bazaar branch in the branch store. | 650 | # Make sure there's a Bazaar branch in the branch store. |
221 | 617 | tree = self.make_branch_and_tree('tree') | 651 | branch = self.make_branch('branch') |
222 | 618 | ImportWorker.pushBazaarWorkingTree(import_worker, tree) | 652 | ImportWorker.pushBazaarBranch(import_worker, branch) |
223 | 619 | # Finally, fetching the tree gets the git.db file too. | 653 | # Finally, fetching the tree gets the git.db file too. |
225 | 620 | tree = import_worker.getBazaarWorkingTree() | 654 | branch = import_worker.getBazaarBranch() |
226 | 621 | self.assertEqual( | 655 | self.assertEqual( |
228 | 622 | content, tree.branch.repository._transport.get('git.db').read()) | 656 | content, branch.repository._transport.get('git.db').read()) |
229 | 623 | 657 | ||
232 | 624 | def test_getBazaarWorkingTree_fetches_git_cache(self): | 658 | def test_getBazaarBranch_fetches_git_cache(self): |
233 | 625 | # GitImportWorker.getBazaarWorkingTree fetches the tarball of the git | 659 | # GitImportWorker.getBazaarBranch fetches the tarball of the git |
234 | 626 | # cache from the worker's ImportDataStore and expands it into the | 660 | # cache from the worker's ImportDataStore and expands it into the |
235 | 627 | # tree's repository. | 661 | # tree's repository. |
236 | 628 | import_worker = self.makeImportWorker() | 662 | import_worker = self.makeImportWorker() |
237 | @@ -633,12 +667,13 @@ | |||
238 | 633 | create_tarball('cache', 'git-cache.tar.gz') | 667 | create_tarball('cache', 'git-cache.tar.gz') |
239 | 634 | import_worker.import_data_store.put('git-cache.tar.gz') | 668 | import_worker.import_data_store.put('git-cache.tar.gz') |
240 | 635 | # Make sure there's a Bazaar branch in the branch store. | 669 | # Make sure there's a Bazaar branch in the branch store. |
243 | 636 | tree = self.make_branch_and_tree('tree') | 670 | branch = self.make_branch('branch') |
244 | 637 | ImportWorker.pushBazaarWorkingTree(import_worker, tree) | 671 | ImportWorker.pushBazaarBranch(import_worker, branch) |
245 | 638 | # Finally, fetching the tree gets the git.db file too. | 672 | # Finally, fetching the tree gets the git.db file too. |
247 | 639 | tree = import_worker.getBazaarWorkingTree() | 673 | new_branch = import_worker.getBazaarBranch() |
248 | 640 | self.assertEqual( | 674 | self.assertEqual( |
250 | 641 | content, tree.branch.repository._transport.get('git/git-cache').read()) | 675 | content, |
251 | 676 | new_branch.repository._transport.get('git/git-cache').read()) | ||
252 | 642 | 677 | ||
253 | 643 | 678 | ||
254 | 644 | def clean_up_default_stores_for_import(source_details): | 679 | def clean_up_default_stores_for_import(source_details): |
255 | 645 | 680 | ||
256 | === modified file 'lib/lp/codehosting/codeimport/worker.py' | |||
257 | --- lib/lp/codehosting/codeimport/worker.py 2010-04-09 04:39:38 +0000 | |||
258 | +++ lib/lp/codehosting/codeimport/worker.py 2010-04-12 01:54:17 +0000 | |||
259 | @@ -64,17 +64,22 @@ | |||
260 | 64 | """Return the URL that `db_branch` is stored at.""" | 64 | """Return the URL that `db_branch` is stored at.""" |
261 | 65 | return urljoin(self.transport.base, '%08x' % db_branch_id) | 65 | return urljoin(self.transport.base, '%08x' % db_branch_id) |
262 | 66 | 66 | ||
265 | 67 | def pull(self, db_branch_id, target_path, required_format): | 67 | def pull(self, db_branch_id, target_path, required_format, |
266 | 68 | """Pull down the Bazaar branch for `code_import` to `target_path`. | 68 | needs_tree=False): |
267 | 69 | """Pull down the Bazaar branch of an import to `target_path`. | ||
268 | 69 | 70 | ||
270 | 70 | :return: A Bazaar working tree for the branch of `code_import`. | 71 | :return: A Bazaar branch for the code import corresponding to the |
271 | 72 | database branch with id `db_branch_id`. | ||
272 | 71 | """ | 73 | """ |
273 | 72 | remote_url = self._getMirrorURL(db_branch_id) | 74 | remote_url = self._getMirrorURL(db_branch_id) |
274 | 73 | try: | 75 | try: |
275 | 74 | remote_bzr_dir = BzrDir.open(remote_url) | 76 | remote_bzr_dir = BzrDir.open(remote_url) |
276 | 75 | except NotBranchError: | 77 | except NotBranchError: |
279 | 76 | return BzrDir.create_standalone_workingtree( | 78 | local_branch = BzrDir.create_branch_and_repo( |
280 | 77 | target_path, required_format) | 79 | target_path, format=required_format) |
281 | 80 | if needs_tree: | ||
282 | 81 | local_branch.bzrdir.create_workingtree() | ||
283 | 82 | return local_branch | ||
284 | 78 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import | 83 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import |
285 | 79 | # branches disabled. Need an orderly upgrade process. | 84 | # branches disabled. Need an orderly upgrade process. |
286 | 80 | if False and remote_bzr_dir.needs_format_conversion( | 85 | if False and remote_bzr_dir.needs_format_conversion( |
287 | @@ -84,33 +89,33 @@ | |||
288 | 84 | except NoSuchFile: | 89 | except NoSuchFile: |
289 | 85 | pass | 90 | pass |
290 | 86 | upgrade(remote_url, required_format) | 91 | upgrade(remote_url, required_format) |
292 | 87 | local_bzr_dir = remote_bzr_dir.sprout(target_path) | 92 | local_branch = remote_bzr_dir.sprout( |
293 | 93 | target_path, create_tree_if_local=needs_tree).open_branch() | ||
294 | 88 | # Because of the way we do incremental imports, there may be revisions | 94 | # Because of the way we do incremental imports, there may be revisions |
295 | 89 | # in the branch's repo that are not in the ancestry of the branch tip. | 95 | # in the branch's repo that are not in the ancestry of the branch tip. |
296 | 90 | # We need to transfer them too. | 96 | # We need to transfer them too. |
298 | 91 | local_bzr_dir.open_repository().fetch( | 97 | local_branch.repository.fetch( |
299 | 92 | remote_bzr_dir.open_repository()) | 98 | remote_bzr_dir.open_repository()) |
301 | 93 | return local_bzr_dir.open_workingtree() | 99 | return local_branch |
302 | 94 | 100 | ||
305 | 95 | def push(self, db_branch_id, bzr_tree, required_format): | 101 | def push(self, db_branch_id, bzr_branch, required_format): |
306 | 96 | """Push up `bzr_tree` as the Bazaar branch for `code_import`. | 102 | """Push up `bzr_branch` as the Bazaar branch for `code_import`. |
307 | 97 | 103 | ||
308 | 98 | :return: A boolean that is true if the push was non-trivial | 104 | :return: A boolean that is true if the push was non-trivial |
309 | 99 | (i.e. actually transferred revisions). | 105 | (i.e. actually transferred revisions). |
310 | 100 | """ | 106 | """ |
311 | 101 | self.transport.create_prefix() | 107 | self.transport.create_prefix() |
312 | 102 | branch_from = bzr_tree.branch | ||
313 | 103 | target_url = self._getMirrorURL(db_branch_id) | 108 | target_url = self._getMirrorURL(db_branch_id) |
314 | 104 | try: | 109 | try: |
316 | 105 | branch_to = Branch.open(target_url) | 110 | remote_branch = Branch.open(target_url) |
317 | 106 | except NotBranchError: | 111 | except NotBranchError: |
319 | 107 | branch_to = BzrDir.create_branch_and_repo( | 112 | remote_branch = BzrDir.create_branch_and_repo( |
320 | 108 | target_url, format=required_format) | 113 | target_url, format=required_format) |
322 | 109 | pull_result = branch_to.pull(branch_from, overwrite=True) | 114 | pull_result = remote_branch.pull(bzr_branch, overwrite=True) |
323 | 110 | # Because of the way we do incremental imports, there may be revisions | 115 | # Because of the way we do incremental imports, there may be revisions |
324 | 111 | # in the branch's repo that are not in the ancestry of the branch tip. | 116 | # in the branch's repo that are not in the ancestry of the branch tip. |
325 | 112 | # We need to transfer them too. | 117 | # We need to transfer them too. |
327 | 113 | branch_to.repository.fetch(branch_from.repository) | 118 | remote_branch.repository.fetch(bzr_branch.repository) |
328 | 114 | return pull_result.old_revid != pull_result.new_revid | 119 | return pull_result.old_revid != pull_result.new_revid |
329 | 115 | 120 | ||
330 | 116 | 121 | ||
331 | @@ -352,7 +357,10 @@ | |||
332 | 352 | """Oversees the actual work of a code import.""" | 357 | """Oversees the actual work of a code import.""" |
333 | 353 | 358 | ||
334 | 354 | # Where the Bazaar working tree will be stored. | 359 | # Where the Bazaar working tree will be stored. |
336 | 355 | BZR_WORKING_TREE_PATH = 'bzr_working_tree' | 360 | BZR_BRANCH_PATH = 'bzr_branch' |
337 | 361 | |||
338 | 362 | # Should `getBazaarBranch` create a working tree? | ||
339 | 363 | needs_bzr_tree = True | ||
340 | 356 | 364 | ||
341 | 357 | required_format = BzrDirFormat.get_default_format() | 365 | required_format = BzrDirFormat.get_default_format() |
342 | 358 | 366 | ||
343 | @@ -372,21 +380,22 @@ | |||
344 | 372 | import_data_transport, self.source_details) | 380 | import_data_transport, self.source_details) |
345 | 373 | self._logger = logger | 381 | self._logger = logger |
346 | 374 | 382 | ||
351 | 375 | def getBazaarWorkingTree(self): | 383 | def getBazaarBranch(self): |
352 | 376 | """Return the Bazaar `WorkingTree` that we are importing into.""" | 384 | """Return the Bazaar `Branch` that we are importing into.""" |
353 | 377 | if os.path.isdir(self.BZR_WORKING_TREE_PATH): | 385 | if os.path.isdir(self.BZR_BRANCH_PATH): |
354 | 378 | shutil.rmtree(self.BZR_WORKING_TREE_PATH) | 386 | shutil.rmtree(self.BZR_BRANCH_PATH) |
355 | 379 | return self.bazaar_branch_store.pull( | 387 | return self.bazaar_branch_store.pull( |
358 | 380 | self.source_details.branch_id, self.BZR_WORKING_TREE_PATH, | 388 | self.source_details.branch_id, self.BZR_BRANCH_PATH, |
359 | 381 | self.required_format) | 389 | self.required_format, self.needs_bzr_tree) |
360 | 382 | 390 | ||
363 | 383 | def pushBazaarWorkingTree(self, bazaar_tree): | 391 | def pushBazaarBranch(self, bazaar_branch): |
364 | 384 | """Push the updated Bazaar working tree to the server. | 392 | """Push the updated Bazaar branch to the server. |
365 | 385 | 393 | ||
366 | 386 | :return: True if revisions were transferred. | 394 | :return: True if revisions were transferred. |
367 | 387 | """ | 395 | """ |
368 | 388 | return self.bazaar_branch_store.push( | 396 | return self.bazaar_branch_store.push( |
370 | 389 | self.source_details.branch_id, bazaar_tree, self.required_format) | 397 | self.source_details.branch_id, bazaar_branch, |
371 | 398 | self.required_format) | ||
372 | 390 | 399 | ||
373 | 391 | def getWorkingDirectory(self): | 400 | def getWorkingDirectory(self): |
374 | 392 | """The directory we should change to and store all scratch files in. | 401 | """The directory we should change to and store all scratch files in. |
375 | @@ -455,14 +464,15 @@ | |||
376 | 455 | os.mkdir(self.FOREIGN_WORKING_TREE_PATH) | 464 | os.mkdir(self.FOREIGN_WORKING_TREE_PATH) |
377 | 456 | return self.foreign_tree_store.fetch(self.FOREIGN_WORKING_TREE_PATH) | 465 | return self.foreign_tree_store.fetch(self.FOREIGN_WORKING_TREE_PATH) |
378 | 457 | 466 | ||
381 | 458 | def importToBazaar(self, foreign_tree, bazaar_tree): | 467 | def importToBazaar(self, foreign_tree, bazaar_branch): |
382 | 459 | """Actually import `foreign_tree` into `bazaar_tree`. | 468 | """Actually import `foreign_tree` into `bazaar_branch`. |
383 | 460 | 469 | ||
384 | 461 | :param foreign_tree: A `SubversionWorkingTree` or a `CVSWorkingTree`. | 470 | :param foreign_tree: A `SubversionWorkingTree` or a `CVSWorkingTree`. |
386 | 462 | :param bazaar_tree: A `bzrlib.workingtree.WorkingTree`. | 471 | :param bazaar_tree: A `bzrlib.branch.Branch`, which must have a |
387 | 472 | colocated working tree. | ||
388 | 463 | """ | 473 | """ |
389 | 464 | foreign_directory = foreign_tree.local_path | 474 | foreign_directory = foreign_tree.local_path |
391 | 465 | bzr_directory = str(bazaar_tree.basedir) | 475 | bzr_directory = str(bazaar_branch.bzrdir.open_workingtree().basedir) |
392 | 466 | 476 | ||
393 | 467 | scm_branch = SCM.branch(bzr_directory) | 477 | scm_branch = SCM.branch(bzr_directory) |
394 | 468 | last_commit = cscvs.findLastCscvsCommit(scm_branch) | 478 | last_commit = cscvs.findLastCscvsCommit(scm_branch) |
395 | @@ -500,9 +510,9 @@ | |||
396 | 500 | 510 | ||
397 | 501 | def _doImport(self): | 511 | def _doImport(self): |
398 | 502 | foreign_tree = self.getForeignTree() | 512 | foreign_tree = self.getForeignTree() |
402 | 503 | bazaar_tree = self.getBazaarWorkingTree() | 513 | bazaar_branch = self.getBazaarBranch() |
403 | 504 | self.importToBazaar(foreign_tree, bazaar_tree) | 514 | self.importToBazaar(foreign_tree, bazaar_branch) |
404 | 505 | non_trivial = self.pushBazaarWorkingTree(bazaar_tree) | 515 | non_trivial = self.pushBazaarBranch(bazaar_branch) |
405 | 506 | self.foreign_tree_store.archive(foreign_tree) | 516 | self.foreign_tree_store.archive(foreign_tree) |
406 | 507 | if non_trivial: | 517 | if non_trivial: |
407 | 508 | return CodeImportWorkerExitCode.SUCCESS | 518 | return CodeImportWorkerExitCode.SUCCESS |
408 | @@ -516,6 +526,8 @@ | |||
409 | 516 | Subclasses need to implement `format_classes`. | 526 | Subclasses need to implement `format_classes`. |
410 | 517 | """ | 527 | """ |
411 | 518 | 528 | ||
412 | 529 | needs_bzr_tree = False | ||
413 | 530 | |||
414 | 519 | @property | 531 | @property |
415 | 520 | def format_classes(self): | 532 | def format_classes(self): |
416 | 521 | """The format classes that should be tried for this import.""" | 533 | """The format classes that should be tried for this import.""" |
417 | @@ -531,13 +543,14 @@ | |||
418 | 531 | return {} | 543 | return {} |
419 | 532 | 544 | ||
420 | 533 | def _doImport(self): | 545 | def _doImport(self): |
424 | 534 | bazaar_tree = self.getBazaarWorkingTree() | 546 | self._logger.info("Starting job.") |
422 | 535 | self.bazaar_branch_store.push( | ||
423 | 536 | self.source_details.branch_id, bazaar_tree, self.required_format) | ||
425 | 537 | saved_factory = bzrlib.ui.ui_factory | 547 | saved_factory = bzrlib.ui.ui_factory |
426 | 538 | bzrlib.ui.ui_factory = LoggingUIFactory( | 548 | bzrlib.ui.ui_factory = LoggingUIFactory( |
427 | 539 | writer=lambda m: self._logger.info('%s', m)) | 549 | writer=lambda m: self._logger.info('%s', m)) |
428 | 540 | try: | 550 | try: |
429 | 551 | self._logger.info( | ||
430 | 552 | "Getting exising bzr branch from central store.") | ||
431 | 553 | bazaar_branch = self.getBazaarBranch() | ||
432 | 541 | transport = get_transport(self.source_details.url) | 554 | transport = get_transport(self.source_details.url) |
433 | 542 | for format_class in self.format_classes: | 555 | for format_class in self.format_classes: |
434 | 543 | try: | 556 | try: |
435 | @@ -549,11 +562,14 @@ | |||
436 | 549 | raise NotBranchError(self.source_details.url) | 562 | raise NotBranchError(self.source_details.url) |
437 | 550 | foreign_branch = format.open(transport).open_branch() | 563 | foreign_branch = format.open(transport).open_branch() |
438 | 551 | foreign_branch_tip = foreign_branch.last_revision() | 564 | foreign_branch_tip = foreign_branch.last_revision() |
440 | 552 | inter_branch = InterBranch.get(foreign_branch, bazaar_tree.branch) | 565 | inter_branch = InterBranch.get(foreign_branch, bazaar_branch) |
441 | 566 | self._logger.info("Importing foreign branch.") | ||
442 | 553 | pull_result = inter_branch.pull( | 567 | pull_result = inter_branch.pull( |
443 | 554 | overwrite=True, **self.getExtraPullArgs()) | 568 | overwrite=True, **self.getExtraPullArgs()) |
446 | 555 | self.pushBazaarWorkingTree(bazaar_tree) | 569 | self._logger.info("Pushing foreign branch to central store.") |
447 | 556 | last_imported_revison = bazaar_tree.branch.last_revision() | 570 | self.pushBazaarBranch(bazaar_branch) |
448 | 571 | last_imported_revison = bazaar_branch.last_revision() | ||
449 | 572 | self._logger.info("Job complete.") | ||
450 | 557 | if last_imported_revison == foreign_branch_tip: | 573 | if last_imported_revison == foreign_branch_tip: |
451 | 558 | if pull_result.old_revid != pull_result.new_revid: | 574 | if pull_result.old_revid != pull_result.new_revid: |
452 | 559 | return CodeImportWorkerExitCode.SUCCESS | 575 | return CodeImportWorkerExitCode.SUCCESS |
453 | @@ -583,39 +599,38 @@ | |||
454 | 583 | """See `PullingImportWorker.getExtraPullArgs`.""" | 599 | """See `PullingImportWorker.getExtraPullArgs`.""" |
455 | 584 | return {'limit': config.codeimport.git_revisions_import_limit} | 600 | return {'limit': config.codeimport.git_revisions_import_limit} |
456 | 585 | 601 | ||
459 | 586 | def getBazaarWorkingTree(self): | 602 | def getBazaarBranch(self): |
460 | 587 | """See `ImportWorker.getBazaarWorkingTree`. | 603 | """See `ImportWorker.getBazaarBranch`. |
461 | 588 | 604 | ||
465 | 589 | In addition to the superclass' behaviour, we retrieve the 'git.db' | 605 | In addition to the superclass' behaviour, we retrieve bzr-git's |
466 | 590 | shamap from the import data store and put it where bzr-git will find | 606 | caches, both legacy and modern, from the import data store and put |
467 | 591 | it in the Bazaar tree, that is at '.bzr/repository/git.db'. | 607 | them where bzr-git will find them in the Bazaar tree, that is at |
468 | 608 | '.bzr/repository/git.db' and '.bzr/repository/git'. | ||
469 | 592 | """ | 609 | """ |
471 | 593 | tree = PullingImportWorker.getBazaarWorkingTree(self) | 610 | branch = PullingImportWorker.getBazaarBranch(self) |
472 | 594 | # Fetch the legacy cache from the store, if present. | 611 | # Fetch the legacy cache from the store, if present. |
473 | 595 | self.import_data_store.fetch( | 612 | self.import_data_store.fetch( |
475 | 596 | 'git.db', tree.branch.repository._transport) | 613 | 'git.db', branch.repository._transport) |
476 | 597 | # The cache dir from newer bzr-gits is stored as a tarball. | 614 | # The cache dir from newer bzr-gits is stored as a tarball. |
477 | 598 | local_name = 'git-cache.tar.gz' | 615 | local_name = 'git-cache.tar.gz' |
478 | 599 | if self.import_data_store.fetch(local_name): | 616 | if self.import_data_store.fetch(local_name): |
480 | 600 | repo_transport = tree.branch.repository._transport | 617 | repo_transport = branch.repository._transport |
481 | 601 | repo_transport.mkdir('git') | 618 | repo_transport.mkdir('git') |
482 | 602 | git_db_dir = os.path.join( | 619 | git_db_dir = os.path.join( |
483 | 603 | local_path_from_url(repo_transport.base), 'git') | 620 | local_path_from_url(repo_transport.base), 'git') |
484 | 604 | extract_tarball(local_name, git_db_dir) | 621 | extract_tarball(local_name, git_db_dir) |
493 | 605 | return tree | 622 | return branch |
494 | 606 | 623 | ||
495 | 607 | def pushBazaarWorkingTree(self, bazaar_tree): | 624 | def pushBazaarBranch(self, bazaar_branch): |
496 | 608 | """See `ImportWorker.pushBazaarWorkingTree`. | 625 | """See `ImportWorker.pushBazaarBranch`. |
497 | 609 | 626 | ||
498 | 610 | In addition to the superclass' behaviour, we store the 'git.db' shamap | 627 | In addition to the superclass' behaviour, we store bzr-git's cache |
499 | 611 | that bzr-git will have created at .bzr/repository/bzr.git into the | 628 | directory at .bzr/repository/git in the import data store. |
492 | 612 | import data store. | ||
500 | 613 | """ | 629 | """ |
506 | 614 | non_trivial = PullingImportWorker.pushBazaarWorkingTree( | 630 | non_trivial = PullingImportWorker.pushBazaarBranch( |
507 | 615 | self, bazaar_tree) | 631 | self, bazaar_branch) |
508 | 616 | repo_transport = bazaar_tree.branch.repository._transport | 632 | repo_base = bazaar_branch.repository._transport.base |
509 | 617 | git_db_dir = os.path.join( | 633 | git_db_dir = os.path.join(local_path_from_url(repo_base), 'git') |
505 | 618 | local_path_from_url(repo_transport.base), 'git') | ||
510 | 619 | local_name = 'git-cache.tar.gz' | 634 | local_name = 'git-cache.tar.gz' |
511 | 620 | create_tarball(git_db_dir, local_name) | 635 | create_tarball(git_db_dir, local_name) |
512 | 621 | self.import_data_store.put(local_name) | 636 | self.import_data_store.put(local_name) |
513 | @@ -637,29 +652,29 @@ | |||
514 | 637 | from bzrlib.plugins.hg import HgBzrDirFormat | 652 | from bzrlib.plugins.hg import HgBzrDirFormat |
515 | 638 | return [HgBzrDirFormat] | 653 | return [HgBzrDirFormat] |
516 | 639 | 654 | ||
519 | 640 | def getBazaarWorkingTree(self): | 655 | def getBazaarBranch(self): |
520 | 641 | """See `ImportWorker.getBazaarWorkingTree`. | 656 | """See `ImportWorker.getBazaarBranch`. |
521 | 642 | 657 | ||
522 | 643 | In addition to the superclass' behaviour, we retrieve the 'hg-v2.db' | 658 | In addition to the superclass' behaviour, we retrieve the 'hg-v2.db' |
523 | 644 | map from the import data store and put it where bzr-hg will find | 659 | map from the import data store and put it where bzr-hg will find |
524 | 645 | it in the Bazaar tree, that is at '.bzr/repository/hg-v2.db'. | 660 | it in the Bazaar tree, that is at '.bzr/repository/hg-v2.db'. |
525 | 646 | """ | 661 | """ |
527 | 647 | tree = PullingImportWorker.getBazaarWorkingTree(self) | 662 | branch = PullingImportWorker.getBazaarBranch(self) |
528 | 648 | self.import_data_store.fetch( | 663 | self.import_data_store.fetch( |
538 | 649 | self.db_file, tree.branch.repository._transport) | 664 | self.db_file, branch.repository._transport) |
539 | 650 | return tree | 665 | return branch |
540 | 651 | 666 | ||
541 | 652 | def pushBazaarWorkingTree(self, bazaar_tree): | 667 | def pushBazaarBranch(self, bazaar_branch): |
542 | 653 | """See `ImportWorker.pushBazaarWorkingTree`. | 668 | """See `ImportWorker.pushBazaarBranch`. |
543 | 654 | 669 | ||
544 | 655 | In addition to the superclass' behaviour, we store the 'hg-v2.db' shamap | 670 | In addition to the superclass' behaviour, we store the 'hg-v2.db' |
545 | 656 | that bzr-hg will have created at .bzr/repository/hg-v2.db into the | 671 | shamap that bzr-hg will have created at .bzr/repository/hg-v2.db into |
546 | 657 | import data store. | 672 | the import data store. |
547 | 658 | """ | 673 | """ |
550 | 659 | non_trivial = PullingImportWorker.pushBazaarWorkingTree( | 674 | non_trivial = PullingImportWorker.pushBazaarBranch( |
551 | 660 | self, bazaar_tree) | 675 | self, bazaar_branch) |
552 | 661 | self.import_data_store.put( | 676 | self.import_data_store.put( |
554 | 662 | self.db_file, bazaar_tree.branch.repository._transport) | 677 | self.db_file, bazaar_branch.repository._transport) |
555 | 663 | return non_trivial | 678 | return non_trivial |
556 | 664 | 679 | ||
557 | 665 | 680 |
The change looks fine.
It does get me wondering a bit why we didn't seem to get any logging when it was pushing the branch to the central store before. Thanks for adding the job started and finished logging bits too.
I agree that it makes more sense to use branch rather than working tree in the workers.