[step 12] request: 12 steps towards a high performance server

[John Arbash Meinel]

John Arbash Meinel wrote:
> ...
> 
>>    http://people.ubuntu.com/~andrew/bzr/remote-bzrdir/
>>      Adds a RemoteBzrDir class, etc, that simply proxies methods through to a
>>      real underlying bzrdir, and hooks the RemoteBzrDir and SmartTransport into
>>      the test suite.
>>
> 
> And finally, we reach the end of the 12-step program. This is another
> big one, though. So I'm sending the diff, and then I'll send another
> reply with the diff inline.
> 
> John
> =:->
> 

...


v-- Similar comments about these being on separate lines
> +
> +from bzrlib import bzrdir, branch, errors, repository
> +from bzrlib.bzrdir import BzrDir, BzrDirFormat
> +from bzrlib.branch import Branch, BranchFormat
> +from bzrlib.trace import mutter
> +from bzrlib.transport.smart import SmartTransport
> +

...

v- Similar comments here. Also need an extra line after the imports
before the 'class'.

> +
> +from bzrlib import bzrdir, remote, tests
> +from bzrlib.transport import smart
> +from bzrlib.bzrdir import BzrDir, BzrDirFormat
> +from bzrlib.remote import RemoteBzrDir, RemoteBzrDirFormat
> +from bzrlib.branch import Branch
> +
> +class BasicRemoteObjectTests(tests.TestCaseInTempDir):
> +
> +    def setUp(self):

...

v- What is raising NoSuchFile rather than NotBranchError?

> +        try:
> +            transport = a_bzrdir.get_branch_transport(None)
> +            control_files = LockableFiles(transport, 'lock', lockdir.LockDir)
> +            return BzrBranch5(_format=self,
> +                              _control_files=control_files,
> +                              a_bzrdir=a_bzrdir,
> +                              _repository=a_bzrdir.find_repository())
> +        except NoSuchFile:
> +            raise NotBranchError(path=transport.base)
>  
>      def __str__(self):
>          return "Bazaar-NG Metadir branch format 5"
...
v- I would avoid using herringbone markers (>>>>>) because we use them
for conflicts. I frequently use this in vim:
/<<<<\|====\|>>>>

Which lets me jump between the conflicted sections, along with 'V' it
makes it really easy to delete one section in favor of the other.

You can use ########## If you like. Or even ## vvvvvvvvv, and matching
## ^^^^^^^.


> +    ## >>>>>>>
> +    # XXX:
> +    # This will always add the tests for smart server transport, regardless of
> +    # the --transport option the user specified to 'bzr selftest'.
> +    from bzrlib.transport.smart import SmartTCPServer_for_testing
> +    from bzrlib.remote import RemoteBzrDirFormat, RemoteRepositoryFormat
> +    from bzrlib.repository import RepositoryFormatKnit1
> +
> +    transport_server = SmartTCPServer_for_testing
> +    smart_server_suite = TestSuite()
> +    adapt_to_smart_server = BzrDirTestProviderAdapter(
> +            transport_server,
> +            None,
> +            [(RemoteBzrDirFormat())])
> +    adapt_modules(test_bzrdir_implementations,
> +                  adapt_to_smart_server,
> +                  TestLoader(),
> +                  smart_server_suite)
> +    result.addTests(smart_server_suite)
> +    ## >>>>>>>
> +
>      return result
> 

...
v- I thought we discussed this, and thought that not all sections should
be in the try/except block. Or did we find that old All-in-one formats
would fail non the bzrdir_format.initialize().

> -        t = get_transport('.')
> -        made_control = self.bzrdir_format.initialize(t.base)
> -        made_repo = made_control.create_repository()
> -        made_branch = made_control.create_branch()
> -        made_tree = made_control.create_workingtree()
> +        t = self.get_transport()
> +        try:
> +            made_control = self.bzrdir_format.initialize(t.base)
> +            made_repo = made_control.create_repository()
> +            made_branch = made_control.create_branch()
> +            made_tree = made_control.create_workingtree()
> +        except errors.NotLocalUrl:
> +            raise TestSkipped("Can't initialize %r on transport %r"
> +                              % (self.bzrdir_format, t))
>          opened_tree = made_control.open_workingtree()
>          self.assertEqual(made_control, opened_tree.bzrdir)
>          self.failUnless(isinstance(opened_tree, made_tree.__class__))
> 

v- I don't really prefer the switch to .has_revision('foo'), but I think
I understand why. I'm not sure if there is something better we could do.

>          made_repo.set_make_working_trees(False)
>          result = made_control.clone(self.get_url('target'))
> -        self.failUnless(isinstance(made_repo, repository.Repository))
> +        # Check that we have a repository object.
> +        made_repo.has_revision('foo')
> +

...

v- So why do you prefer to create multiple transports, rather than
creating it 1 time, and just re-using it?

>      def create_test_bundle(self):
> -        # Can't use self.get_transport() because that asserts that 
> -        # it is not readonly, so just skip tests where the server is readonly
> -        self._transport = self.get_transport()
> -        #if isinstance(self._transport, MemoryTransport):
> -        #    return None
>          self.build_tree(['tree/', 'tree/a', 'tree/subdir/'])
>  
>          format = bzrlib.bzrdir.BzrDirFormat.get_default_format()
>  
> -
>          bzrdir = format.initialize('tree')
>          repo = bzrdir.create_repository()
>          branch = repo.bzrdir.create_branch()
> @@ -59,12 +53,12 @@
>          rev_ids = write_bundle(wt.branch.repository,
>                                 wt.get_parent_ids()[0], None, out)
>          out.seek(0)
> -        if self._transport.is_readonly():
> +        if self.get_transport().is_readonly():
>              f = open('test_bundle', 'wb')
>              f.write(out.getvalue())
>              f.close()
>          else:
> -            self._transport.put_file('test_bundle', out)
> +            self.get_transport().put_file('test_bundle', out)
>              self.log('Put to: %s', self.get_url('test_bundle'))
>          return wt
>  


Overall, this seems like a pretty straightforward implementation. So I
think it is pretty good.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060913/77479acd/attachment.pgp