[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