[MERGE]make readv(adjust_for_latency) work better at the end of files.

Robert Collins robertc at robertcollins.net
Thu Oct 4 05:19:03 BST 2007


On Wed, 2007-10-03 at 23:42 -0400, Martin Pool wrote:
> Martin Pool has voted tweak.
> Status is now: Conditionally approved
> Comment:
> +This does not change the transport behaviour at allmerely records every 
> call
> 
> "at all, merely"

Thanks

> +
> +class TransportTraceDecorator(TransportDecorator):
> +    """A tracing decorator for Transports.
> +
> +    Calls that potentially perform IO are logged to self._activity. The
> +    _activity attribute is shared as the transport is cloned, but not 
> if a new
> +    transport is created without cloning.
> +    """
> 
> That's a useful thing to have.  I think the name and url syntax is a bit 
> misleading though as it doesn't actually trace them (to bzrlib.trace) 
> but rather just records them.  So, unless you're planning to make it 
> trace them too(?), how about renaming it to 'record' or something else?
> 
> Add a comment inviting people to record other methods too?

I'll definately add a comment. I think it the name record is no more or
less misleading; and this transport can be tweaked to be something that
traces to bzrlib.trace, in future changes.

> +    def append_file(self, relpath, f, mode=None):
> +        """See Transport.append_file()."""
> +        return self._decorated.append_file(relpath, f, mode=mode)
> +
> +    def append_bytes(self, relpath, bytes, mode=None):
> +        """See Transport.append_bytes()."""
> +        return self._decorated.append_bytes(relpath, bytes, mode=mode)
> +
> 
> I don't understand why these are here, they seem redundant with the 
> definitions in TransportDecorator.

I pulled across the methods that will need to be adjusted to record all
operations, but have only instrumented those that I had time to write
tests for the instrumentation of - and needed.

> 
> +def get_test_permutations():
> +    """Return the permutations to be used in testing.
> +
> +    The Decorator class is not directly usable, and testing it would 
> not have
> +    any benefit - its the concrete classes which need to be tested.
> +    """
> +    return [(TransportTraceDecorator, TraceServer)]
> 
> The docstring seems to contradict the code.

Copy and paste glitch, shall correct.

> === modified file 'NEWS'
> 
> You should mention the new decorator as something people can use in 
> testing.

Thanks.

> >  * Give Registry a useful __repr__.
> 
> You seem to have added one to _LazyObjectGetter, not to Registry.

Thanks.

> +        del instrumented_t._activity[:]
> 
> This seems like one of the weirder bits of Python syntax (compared to 
> a[:] = []), but it's correct.

Would you prefer a[:] = [] ?

> -def get_test_permutations():
> -    """Return transport permutations to be used in testing.
> -
> -    This module registers some transports, but they're only for testing
> -    registration.  We don't really want to run all the transport tests 
> against
> -    them.
> -    """
> -    return []
> 
> You didn't say why you deleted this.

It was bogus, it only existed because of the bug with polluted transport
space.

> +        transport2 = transport.clone('.')
> +        self.assertTrue(transport is not transport2)
> +        self.assertTrue(transport._activity is transport2._activity)
> 
> I guess you have the first assertion to make sure that the second one is 
> a worthwhile test.  But I'm not sure that clone('.') is actually 
> required to return a new object, rather than it being accidental here. 
> I would have tested a transport in a subdir.

Its not required to at the interface level, but this transport does, and
this is specific to this transport.

> -class TransportLogger(object):
> 
> removal should be in news?  I guess it's unlikely anyone will care.

Will add.

...
> If you would just clear up those bits and decide with me on whether to 
> rename it or not I'm happy for this to merge.

Hope the above helps.

Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071004/e8bfef78/attachment.pgp 


More information about the bazaar mailing list