[MERGE] [hpss part 2] Use the Command pattern for smart server request handling

[John Arbash Meinel]

I really like the move towards the Command pattern.

As we move forward, I'm just curious if we have a direct test for the
wire-level details of these commands. Just to make sure that as we
'upgrade' we don't break compatibility between versions. I'm not asking
you to do so, just to confirm that we have (or don't have) it.


Andrew Bennetts wrote:
> This is the next part of merging the hpss branch into bzr.dev.
> 
> This bundle splits server processing of smart requests into individual command
> objects (using a standard Registry), rather than dispatching to do_* methods on
> SmartServerRequestHandler.  A nice benefit of this is that requests can be
> grouped into modules, rather than all living on one extremely large class.
> 
> This bundle doesn't add any new requests (like the previous hpss bundle, this
> is basically just a restructuring), but later ones will.  It also includes a
> simple facility to disable the "VFS" smart requests by use of an environment
> variable, which is intended as a development aid rather than a production
> feature.
> 
> -Andrew.
> 

...

> === added file bzrlib/smart/vfs.py // file-id:vfs.py-20061108095550-gunadhxmzkd
> ... jfeek-2
> --- /dev/null
> +++ bzrlib/smart/vfs.py
> @@ -0,0 +1,198 @@

v- Copyright should include 2007.

> +# Copyright (C) 2006 Canonical Ltd
> +#

...

> +def vfs_enabled():
> +    """Is the VFS enabled ?
> +
> +    the VFS is disabled when the NO_SMART_VFS environment variable is set.
> +
> +    :return: True if it is enabled.
> +    """
> +    return not 'NO_SMART_VFS' in os.environ

^- Any bzr related env var should probably be "BZR_*" so BZR_NO_SMART_VFS.


> +

...


v- Why did you downgrade to 2006?

> === modified file bzrlib/smart/request.py
> --- bzrlib/smart/request.py
> +++ bzrlib/smart/request.py
> @@ -1,4 +1,4 @@
> -# Copyright (C) 2006, 2007 Canonical Ltd
> +# Copyright (C) 2006 Canonical Ltd
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -19,14 +19,54 @@
>  
>  import tempfile


v- This also seems to be a "regression". In as much as you are more
likely to get conflicts from updates to a:

from foo import a, b, c, d, e

rather than

from foo import (
  a,
  b,
  c,
  d,
  )

Also, you can trivially keep the latter sorted in Vim by selecting the
lines, and running :!sort

> -from bzrlib import (
> -    bzrdir,
> -    errors,
> -    revision
> -    )
> +from bzrlib import bzrdir, errors, registry, revision
>  from bzrlib.bundle.serializer import write_bundle
>  
>  

v- Single line doc strings close the quotes on the same line.

> +class SmartServerRequest(object):
> +    """Base class for request handlers.
> +    """
> +

v- We have a history of not documenting __init__, but all of the
documentation tools seem to complain about that. I'm usually not sure
what to put there, but I suppose at least a description of ':param:' is
useful. (minor)

> +    def __init__(self, backing_transport):
> +        self._backing_transport = backing_transport
> +
> +    def _check_enabled(self):
> +        """Raises DisabledMethod if this method is disabled."""
> +        pass
> +
> +    def do(self, *args):
> +        """Mandatory extension point for SmartServerRequest subclasses.
> +        
> +        Subclasses must implement this.
> +        
> +        This should return a SmartServerResponse if this command expects to
> +        receive no body.
> +        """
> +        raise NotImplementedError(self.do)
> +
> +    def execute(self, *args):
> +        """Public entry point to execute this request.
> +
> +        It will return a SmartServerResponse if the command does not expect a
> +        body.
> +
> +        :param *args: the arguments of the request.
> +        """
> +        self._check_enabled()
> +        return self.do(*args)
> +
> +    def do_body(self, body_bytes):
> +        """Called if the client sends a body with the request.
> +        
> +        Must return a SmartServerResponse.
> +        """
> +        # TODO: if a client erroneously sends a request that shouldn't have a
> +        # body, what to do?  Probably SmartServerRequestHandler should catch
> +        # this NotImplementedError and translate it into a 'bad request' error
> +        # to send to the client.
> +        raise NotImplementedError(self.do_body)
> +
> +
...

v- Docstring?

> +class GetBundleRequest(SmartServerRequest):
> +
> +    def do(self, path, revision_id):
> +        # open transport relative to our base
> +        t = self._backing_transport.clone(path)
> +        control, extra_path = bzrdir.BzrDir.open_containing_from_transport(t)
> +        repo = control.open_repository()
> +        tmpf = tempfile.TemporaryFile()
> +        base_revision = revision.NULL_REVISION
> +        write_bundle(repo, revision_id, base_revision, tmpf)
> +        tmpf.seek(0)
> +        return SmartServerResponse((), tmpf.read())
> +
> +

v- Maybe we should have a naming structure for how to handle
"development" verbs. Like starting them with an '_' :)

> +# This exists solely to help RemoteObjectHacking.  It should be removed
> +# eventually.  It should not be considered part of the real smart server
> +# protocol!
> +class ProbeDontUseRequest(SmartServerRequest):
> +
> +    def do(self, path):
> +        from bzrlib.bzrdir import BzrDirFormat
> +        t = self._backing_transport.clone(path)
> +        default_format = BzrDirFormat.get_default_format()
> +        real_bzrdir = default_format.open(t, _found=True)
> +        try:
> +            real_bzrdir._format.probe_transport(t)
> +        except (errors.NotBranchError, errors.UnknownFormatError):
> +            answer = 'no'
> +        else:
> +            answer = 'yes'
> +        return SmartServerResponse((answer,))
> +

v- If you update the env var, don't forget about here.

> +    def setUp(self):
> +        super(TestSmartServerStreamMedium, self).setUp()
> +        self._captureVar('NO_SMART_VFS', None)
> +
>      def portable_socket_pair(self):
>          """Return a pair of TCP sockets connected to each other.
>          
> @@ -780,6 +785,7 @@
>  

v- If you are doing it in 'setUp()' do you need to do it in each test?

>      def test_get_error_unexpected(self):
>          """Error reported by server with no specific representation"""
> +        self._captureVar('NO_SMART_VFS', None)
>          class FlakyTransport(object):
>              base = 'a_url'
>              def get_bytes(self, path):
> @@ -865,12 +871,14 @@
>  
>      def test_smart_transport_has(self):
>          """Checking for file existence over smart."""
> +        self._captureVar('NO_SMART_VFS', None)
>          self.backing_transport.put_bytes("foo", "contents of foo\n")
>          self.assertTrue(self.transport.has("foo"))
>          self.assertFalse(self.transport.has("non-foo"))
>  
>      def test_smart_transport_get(self):
>          """Read back a file over smart."""
> +        self._captureVar('NO_SMART_VFS', None)
>          self.backing_transport.put_bytes("foo", "contents\nof\nfoo\n")
>          fp = self.transport.get("foo")
>          self.assertEqual('contents\nof\nfoo\n', fp.read())
> @@ -880,6 +888,7 @@
>          # The path in a raised NoSuchFile exception should be the precise path
>          # asked for by the client. This gives meaningful and unsurprising errors
>          # for users.
> +        self._captureVar('NO_SMART_VFS', None)
>          try:
>              self.transport.get('not%20a%20file')
>          except errors.NoSuchFile, e:
> @@ -906,6 +915,7 @@
>  
>      def test_open_dir(self):
>          """Test changing directory"""
> +        self._captureVar('NO_SMART_VFS', None)
>          transport = self.transport
>          self.backing_transport.mkdir('toffee')
>          self.backing_transport.mkdir('toffee/apple')
> @@ -933,6 +943,7 @@
>  
>      def test_mkdir_error_readonly(self):
>          """TransportNotPossible should be preserved from the backing transport."""
> +        self._captureVar('NO_SMART_VFS', None)
>          self.setUpServer(readonly=True)
>          self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,
>              'foo')
> @@ -977,19 +988,16 @@
>  # server-stopped hook.
>  

Otherwise +1.

John
=:->