[MERGE] Better infrastructure for tracking remote server version in SmartClientMedium
andrew at canonical.com
Thu Jun 12 05:35:01 BST 2008
Martin Pool wrote:
> On Wed, Jun 11, 2008 at 3:05 PM, Andrew Bennetts <andrew at canonical.com> wrote:
> > I don't much like these method names much. I'm open to suggestions for
> > improvement.
> The other thing that was bothering me is that I think the client
> actually knows the server is in a particular interval of versions: at
> least X, but no more than Y. These methods seem a bit confused about
Hmm, I suppose that's true. But we only care about the upper bound, so that's
all I'm tracking at the moment.
> > Well, we need some way to gracefully cope with newer clients talking to older
> > servers. The current approach of just optimistically trying the highest version
> > then falling back, and remember that you had to fall back on a per-connection
> > basis, seems to be the least worst option.
> > We could remember that individual verbs aren't available, but that seems like a
> > YAGNI to me, especially considering it would add complexity to give worse
> > performance (if one 1.2 verb is not supported, why waste time trying another?).
> > An added benefit of this approach is that it makes it easy to say "if the
> > protocol version is 2, we can immediately assume the remote only supports verbs
> > from 1.2 and earlier". It also makes it easy to add a
> > Remote-objects-with-a-no-1.6-verbs-server scenario to
> > bzrdir/branch/repository-implementations along side the existing
> > Remote-objects-with-current-server scenario (I have a patch to add exactly that
> > that I'm about to submit now that I've got tests passing in a dependent loom
> > thread).
> Yes this does seem to touch on systematically knowing which version
> added which verbs and testing across the facilities available in
> earlier versions.
> If we are going to remember the server's version as a number across
> replies maybe it's better to just make use of the fact that in
> protocol 3 it's given explicitly, rather than ignoring that and
> inferring it?
We still need to fallback at least on the very first request. Which means that
we either need approximately the same level of complexity as we have now
(fallback logic built-in to any Remote* method that might need it), or we go
back to making a “hello” request of some sort an relying exclusively on that.
We'd have to think carefully about how version information is sent over the wire
if we go back to a “hello”-like strategy. The existing “Software version”
header isn't appropriate (we don't really want to get into parsing and
comparing strings like "1.3.1" and "1.6b2"). We could add a “Protocol version”
header, but that's a small inconvenience to the test suite when generating
protocol requests/responses by hand and generally cluttering the bytes of every
request. Or we could add new “hello” verb that doesn't have the flaws of the
old one (i.e. make the client realise that a newer server is fine).
I'm happy with the existing fallback strategy: it doesn't penalise you at all if
both client and server are up to date, and the maintenance burden doesn't seem
to be onerous (we aren't regularly finding bugs in the fallback logic, and it
doesn't impede my work on that code significantly).
I guess what I'm saying is the patch I've proposed seems to satisfy all our
current and anticipated use-cases adequately, and is a simple improvement over
what we are doing already. If I change _is_remote_at_least() to a read-only
property called something like “_remote_version_guess” that callers can do <=
comparisons on, are you happy with that?
More information about the bazaar