[MERGE] Updated sftp_readv
Vincent Ladeuil
v.ladeuil+lp at free.fr
Thu Dec 20 16:27:13 GMT 2007
>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:
john> Vincent Ladeuil wrote:
john> ...
>> I think we should take a decision on that point and also
>> includes/excludes the ability to specify duplicate offsets to
>> readv.
>>
>> I can't imagine use cases for neither functionality, so I'd vote
>> to get rid of them.I f needed, the caller can handle such cases
>> but at the transport level and given the complexity of the
>> actuals implementationS, I'd prefer that we simply exclude them.
john> Well, I did set the default to allow_overlap=False. And
john> it turns out that the overlap check will also disallow
john> duplicate ranges.
True. So what about getting rid of that, if the need arise,
either the caller may find a simple way to do it or we'll have a
cleaner base to implement it.
john> ...
john> last_end = None
john> cur = _CoalescedOffset(None, None, [])
john> + coalesced_offsets = []
>>
>> Why did you delete the iterator behavior ? You still iterate the
>> result in the caller.
john> Because all callers actually use a list. If you look at
john> the sftp code, it actually casts it into a list, and
john> wraps a different iterator around it. We need the data
john> 2 times, so we have to use a list. The second iterator
john> is just rather than keeping an integer of which offset
john> we are on.
Ok. I was just surprised, no problem with that.
<snip/>
john> Are you talking about _coalesce_offsets or _sftp_readv?
_sftp_readv.
_coalesce_offsets is a bit surprising at first read, but after
that, there is one big condition which is easy to read.
<snip/>
>> Your complexity daemon warns you too I see :)
john> Actually, I had started to try and implement it that
john> way. It was going to take me too long to get all the
john> bits right, and doing "data = buffer[start:end]" is a
john> whole lot simpler than trying to do the same thing from
john> a list.
Sure. But l.append() / s = ''.join(l) is mentioned in several
places as the most optimal way to handle such things, but until
the method become far simpler I will not require that change.
<snip/>
john> It shouldn't be possible from the outside. So it isn't something we can
john> actually test.
john> I could take the if statement out completely, though it
john> did help before I got the algorithm correct.
My thought.
If the method was split it may become possible to test from
outside, that's what I was trying to express (poorly).
<snip/>
john> For HTTP you pushed the complexity down into a helper (response file). Which
john> might be a reasonable way to handle it here.
Exactly.
<snip/>
john> + buffered_data = ''
john> + buffered_start = cur_coalesced.start
john> + elif buffered_end >= cur_coalesced.start + cur_coalesced.length:
john> + raise AssertionError('Somehow we read too much data: %s > %s'
john> + % (buffered_end, cur_coalesced.start + cur_coalesced.length))
>>
>> Same here, if that could be documented and exercised in a test,
>> the resulting code would be clearer.
john> Well, the test was already present, and it is something that depends on
john> "fp.readv()" returning more data than we requested. Which would be the fault of
john> Paramiko.
Ok.
john> I'm not really sure how you would document and exercise
john> it in a test. But if you really did want to test it,
john> then you need code to handle it. Which means that you
john> still have this elif and raise an exception here.
john> So I'm not sure how it would simplify the cdoe.
Forget it, I misunderstood. A comment mentioning your explanation
above (even shorter) will be enough.
<snip/>
>> In summary, on three main criteria:
>> - Doesn't reduce test coverage
>> - Doesn't reduce design clarity
>> - Improves bugs, features, speed, or code simplicity.
>>
>> Err, I think you succeed only on part of the third: improve bugs
>> ;-)
john> Well, I would argue it doesn't improve a bug, but features/speed. :)
Well I copy/pasted from HACKING and was a bit puzzled by the
'improve bug'.
If I would argue I'd say your patch improve the robustness by
putting less stress on memory, so yes improving features/speed by
not trashing/crashing ;-)
>>
>> Hope that helps,
>>
>> Vincent
>>
>> P.S.: Since this is my first real review, don't hesitate to
>> comment the review itself.
>>
john> I think you did very well. You also remembered to think
john> outside of just what was written, which is what I find
john> the hardest to do when reviewing.
Thanks,
Vincent
More information about the bazaar
mailing list