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

Martin Pool mbp at canonical.com
Thu Oct 4 04:42:39 BST 2007


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"

+
+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?

+    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.


+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.

=== modified file 'NEWS'

You should mention the new decorator as something people can use in 
testing.

>  * Give Registry a useful __repr__.

You seem to have added one to _LazyObjectGetter, not to Registry.

+        del instrumented_t._activity[:]

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

-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.

+        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.

-class TransportLogger(object):

removal should be in news?  I guess it's unlikely anyone will care.

+            # expand by page size at either end
+            expansion = self.recommended_page_size()
+            reduction = expansion / 2

I would have used expansion//2 there (works in python2.4), though it's 
unlikely it would ever be odd.

 From the name page size I would have expected we'd be trying to get ios 
that are aligned modulo that size, not just expanded by that size.  For 
local disk, that may be more efficient.  For remote transports it's 
probably not noticeable.

+            # combine the expanded offsets

I thought there was some existing code that would do this, but maybe 
not.

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.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1191303994.14243.2.camel%40lifeless-64%3E



More information about the bazaar mailing list