Bazaar

Merge lp:~mbp/bzr/456077-cross-format-fetch into lp:bzr/2.0

  1. 456077-cross-format-fetch
  2. Merge into 2.0
Proposed by Martin Pool
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/456077-cross-format-fetch
Merge into: lp:bzr/2.0
Diff against target: 188 lines (+69/-21)
5 files modified
NEWS (+3/-3)
bzrlib/repository.py (+8/-7)
bzrlib/smart/repository.py (+9/-1)
bzrlib/tests/blackbox/test_pull.py (+41/-9)
bzrlib/ui/__init__.py (+8/-1)
To merge this branch: bzr merge lp:~mbp/bzr/456077-cross-format-fetch
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+17354@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

* Give warnings about cross-format fetch for pushes and pulls across the network.
* Put the actual warning into the UI object.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/456077-cross-format-fetch into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #456077 MYSQL/BZR P3: bzr doesn't explain it's doing a slow cross-format fetch
> https://bugs.launchpad.net/bugs/456077
>
>
> * Give warnings about cross-format fetch for pushes and pulls across the network.
> * Put the actual warning into the UI object.
>
>

This seems ok. My main concern is that I don't see a dedup counter. So
if there are multiple 'inventory-delta' substreams (allowed by the
protocol, may not happen in practice) then you'll get the warning
multiple times.

Otherwise the change to a common warning helper is nice. I guess on
ui_factory is reasonable as long as it is on the base class so that
other factories don't *have* to implement it. (potentially a 2.0-stable
compatibility issue.)

"Upgrade the branches" also isn't 100% accurate, given that it is the
repository format that matters...

I'm happy enough with this, but you may want to tweak it before landing.

 review: approve
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktPLQIACgkQJdeBCYSNAAP7IQCdG/GXeDjqN6sFzukMpnluf0/U
SsQAniso3Xu+z/pjbvaFhnS0yftZr7ga
=dfAR
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

> This seems ok. My main concern is that I don't see a dedup counter. So
> if there are multiple 'inventory-delta' substreams (allowed by the
> protocol, may not happen in practice) then you'll get the warning
> multiple times.

It's a fair point, but it doesn't seem to happen at the moment.

Should the de-duping be local? Maybe the warning suppression code should allow it to be global just once, as for Python warnings.

> "Upgrade the branches" also isn't 100% accurate, given that it is the
> repository format that matters...

Good point.

>
> I'm happy enough with this, but you may want to tweak it before landing.

I'll just correct the phrasing and send it. Bug 507655 is a followon for being able to suppress the warning.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-01-13 16:31:34 +0000
3+++ NEWS 2010-01-15 03:36:28 +0000
4@@ -41,10 +41,10 @@
5 * Give a clearer message if the lockdir disappears after being apparently
6 successfully taken. (Martin Pool, #498378)
7
8-* Give a warning when fetching between local repositories with
9+* Give a warning when fetching between repositories (local or remote) with
10 sufficiently different formats that the content will need to be
11- serialized (ie ``InterDifferingSerializer``) so the user has a clue that
12- upgrading could make it faster.
13+ serialized (ie ``InterDifferingSerializer`` or ``inventory-deltas``), so
14+ the user has a clue that upgrading could make it faster.
15 (Martin Pool, #456077)
16
17 * If we fail to open ``~/.bzr.log`` write a clear message to stderr rather
18
19=== modified file 'bzrlib/repository.py'
20--- bzrlib/repository.py 2010-01-12 06:12:31 +0000
21+++ bzrlib/repository.py 2010-01-15 03:36:28 +0000
22@@ -3984,11 +3984,8 @@
23 #
24 # nb this is only active for local-local fetches; other things using
25 # streaming.
26- trace.warning("Fetching between repositories with different formats\n"
27- "from %s to %s.\n"
28- "This may take some time. Upgrade the branches to the same format \n"
29- "for better results.\n"
30- % (self.source._format, self.target._format))
31+ ui.ui_factory.warn_cross_format_fetch(self.source._format,
32+ self.target._format)
33 if (not self.source.supports_rich_root()
34 and self.target.supports_rich_root()):
35 self._converting_to_rich_root = True
36@@ -4287,6 +4284,8 @@
37 self._extract_and_insert_inventories(
38 substream, src_serializer)
39 elif substream_type == 'inventory-deltas':
40+ ui.ui_factory.warn_cross_format_fetch(src_format,
41+ self.target_repo._format)
42 self._extract_and_insert_inventory_deltas(
43 substream, src_serializer)
44 elif substream_type == 'chk_bytes':
45@@ -4591,8 +4590,10 @@
46
47 def _get_convertable_inventory_stream(self, revision_ids,
48 delta_versus_null=False):
49- # The source is using CHKs, but the target either doesn't or it has a
50- # different serializer. The StreamSink code expects to be able to
51+ # The two formats are sufficiently different that there is no fast
52+ # path, so we need to send just inventorydeltas, which any
53+ # sufficiently modern client can insert into any repository.
54+ # The StreamSink code expects to be able to
55 # convert on the target, so we need to put bytes-on-the-wire that can
56 # be converted. That means inventory deltas (if the remote is <1.19,
57 # RemoteStreamSink will fallback to VFS to insert the deltas).
58
59=== modified file 'bzrlib/smart/repository.py'
60--- bzrlib/smart/repository.py 2009-09-03 00:33:35 +0000
61+++ bzrlib/smart/repository.py 2010-01-15 03:36:28 +0000
62@@ -1,4 +1,4 @@
63-# Copyright (C) 2006, 2007 Canonical Ltd
64+# Copyright (C) 2006, 2007, 2010 Canonical Ltd
65 #
66 # This program is free software; you can redistribute it and/or modify
67 # it under the terms of the GNU General Public License as published by
68@@ -30,6 +30,7 @@
69 graph,
70 osutils,
71 pack,
72+ ui,
73 versionedfile,
74 )
75 from bzrlib.bzrdir import BzrDir
76@@ -502,6 +503,13 @@
77 yield pack_writer.begin()
78 yield pack_writer.bytes_record(src_format.network_name(), '')
79 for substream_type, substream in stream:
80+ if substream_type == 'inventory-deltas':
81+ # This doesn't feel like the ideal place to issue this warning;
82+ # however we don't want to do it in the Repository that's
83+ # generating the stream, because that might be on the server.
84+ # Instead we try to observe it as the stream goes by.
85+ ui.ui_factory.warn_cross_format_fetch(src_format,
86+ '(remote)')
87 for record in substream:
88 if record.storage_kind in ('chunked', 'fulltext'):
89 serialised = record_to_fulltext_bytes(record)
90
91=== modified file 'bzrlib/tests/blackbox/test_pull.py'
92--- bzrlib/tests/blackbox/test_pull.py 2010-01-13 16:27:22 +0000
93+++ bzrlib/tests/blackbox/test_pull.py 2010-01-15 03:36:28 +0000
94@@ -1,4 +1,4 @@
95-# Copyright (C) 2005-2010 Canonical Ltd
96+# Copyright (C) 2005, 2006, 2010 Canonical Ltd
97 #
98 # This program is free software; you can redistribute it and/or modify
99 # it under the terms of the GNU General Public License as published by
100@@ -20,13 +20,18 @@
101 import os
102 import sys
103
104+from bzrlib import (
105+ debug,
106+ remote,
107+ urlutils,
108+ )
109+
110 from bzrlib.branch import Branch
111 from bzrlib.directory_service import directories
112 from bzrlib.osutils import pathjoin
113 from bzrlib.tests.blackbox import ExternalBase
114 from bzrlib.uncommit import uncommit
115 from bzrlib.workingtree import WorkingTree
116-from bzrlib import urlutils
117
118
119 class TestPull(ExternalBase):
120@@ -394,10 +399,37 @@
121 def test_pull_cross_format_warning(self):
122 """You get a warning for probably slow cross-format pulls.
123 """
124-
125- from_tree = self.make_branch_and_tree('from', format='2a')
126- to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
127- from_tree.commit(message='first commit')
128- out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
129- self.assertContainsRe(err,
130- "(?m)Fetching between repositories with different formats.*")
131+ # this is assumed to be going through InterDifferingSerializer
132+ from_tree = self.make_branch_and_tree('from', format='2a')
133+ to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
134+ from_tree.commit(message='first commit')
135+ out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
136+ self.assertContainsRe(err,
137+ "(?m)Doing on-the-fly conversion")
138+
139+ def test_pull_cross_format_warning_no_IDS(self):
140+ """You get a warning for probably slow cross-format pulls.
141+ """
142+ # this simulates what would happen across the network, where
143+ # interdifferingserializer is not active
144+
145+ debug.debug_flags.add('IDS_never')
146+ # TestCase take care of restoring them
147+
148+ from_tree = self.make_branch_and_tree('from', format='2a')
149+ to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
150+ from_tree.commit(message='first commit')
151+ out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
152+ self.assertContainsRe(err,
153+ "(?m)Doing on-the-fly conversion")
154+
155+ def test_pull_cross_format_from_network(self):
156+ self.setup_smart_server_with_call_log()
157+ from_tree = self.make_branch_and_tree('from', format='2a')
158+ to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
159+ self.assertIsInstance(from_tree.branch, remote.RemoteBranch)
160+ from_tree.commit(message='first commit')
161+ out, err = self.run_bzr(['pull', '-d', 'to',
162+ from_tree.branch.bzrdir.root_transport.base])
163+ self.assertContainsRe(err,
164+ "(?m)Doing on-the-fly conversion")
165
166=== modified file 'bzrlib/ui/__init__.py'
167--- bzrlib/ui/__init__.py 2009-07-22 07:34:08 +0000
168+++ bzrlib/ui/__init__.py 2010-01-15 03:36:28 +0000
169@@ -1,4 +1,4 @@
170-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
171+# Copyright (C) 2005-2010 Canonical Ltd
172 #
173 # This program is free software; you can redistribute it and/or modify
174 # it under the terms of the GNU General Public License as published by
175@@ -208,6 +208,13 @@
176 """
177 pass
178
179+ def warn_cross_format_fetch(self, from_format, to_format):
180+ """Warn about a potentially slow cross-format transfer"""
181+ # See <https://launchpad.net/bugs/456077> asking for a warning here
182+ trace.warning("Doing on-the-fly conversion from %s to %s.\n"
183+ "This may take some time. Upgrade the repositories to the "
184+ "same format for better performance.\n" %
185+ (from_format, to_format))
186
187
188 class CLIUIFactory(UIFactory):

Subscribers

People subscribed via source and target branches