[MERGE][bug #173010] initial branch over http: can show no ui activity for a very long time

Vincent Ladeuil v.ladeuil+lp at free.fr
Thu Dec 6 10:21:37 GMT 2007


>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> Vincent Ladeuil wrote:
    >> "Gimme my blinkenlights back !" said the user...
    >> 
    >> Here you are.
    >> 
    >> The attached fix is a complete rewrite of the http response
    >> handling.
    Andrew> [...]

    Andrew> This is pretty cool stuff.  I'm going to be a wuss
    Andrew> for the moment and make this vote:

    Andrew> bb:comment

    Andrew> But I intend to change that to something more helpful
    Andrew> shortly :) This is just so you can start dealing with
    Andrew> my comments while I finish thinking about this code.

Thanks a ton, your comments make a lot of sense, especially in
some areas I didn't think more than 'Let's make it work'.

    Andrew> I'm a little bit scared about this for 1.0, to be
    Andrew> honest.  While I think the code is pretty good
    Andrew> quality, I fear that that stressing HTTP servers with
    Andrew> more ambitious range requests is going to encounter
    Andrew> new problems in various HTTP servers and proxies.
    Andrew> But on the other hand it'd be very nice to have
    Andrew> this...

I understand that. I even felt the same when I began working on
it.

Now, you have to take into account that:

- we parse the http responses in a more robust way (modulo some
  (unknown) bugs I may have introduced by changing the data *flow*,
  but I doubt it),

- we now have a better control on the Range header content which
  was the source of the bugs we encounter in the past,

- at worst, we ask http servers to download bigger files, but I
  can't see why that could be a source of problems (if there is a
  limit on file size, we have some serious problem though).

So we should be more gentle now with the http servers than we was
before that change (again modulo the root cause of your squid
issue still unclear for me).

So I'm not scared anymore to include this for 1.0, but I have no
problem with it being delayed to after 1.0. Except that I put
some hard work in it since http may get some bad press about
making bzr slow with packs ;-) But I'm on the "make it works
first" camp, so no hard feelings there.

To summarize it another way:

1) I'm confident in the robustness of urllib (if bugs occurs they
   will be easier to diagnose and fix than before) thanks to -Dhttp,

2) I'm not convinced that pycurl can be hacked to return early,
   by fiddling with the write_header and write_data hooks that
   *could* allow some sort of similar behavior we have with
   urllib. The current plan is to make pycurl issue more requests
   so that we can make the progress bar spin. Of course it's a
   bad solution since more requests mean more latency... I'm not a
   fan of that solution but that's the only available way to
   address the progress bar problem.

3) I'm unsatisfied with pycurl results, the _max_readv_combine
   proposed value is a wild guess and should be tuned, in fact it
   should not even be used, see next point,

4) _coalesce_offsets should be modified (don't know if I'll find
   time for that, but that may be easier than I think) to allow
   http clients to specify a byte limit for requests instead of a
   number of ranges)

5) http needs ranges that minimize the number of requests without
   overflowing the header space while making requests of a
   reasonable amount

The last two points will allow us to go from a model where we do:

  raw ranges => count filter => collapsed ranges => requests

to one where we do

  raw ranges => collapsed ranges => byte count filter => requests

as discussed on IRC with jam and lifeless.

    Andrew> Anyway, here's my initial review.  I've tried to not
    Andrew> to repeat comments John's already made.

Since you were kind enough to point my typos, perhaps you can
enlighten me about "tried to not to repeat", is that correct ? I
would have used "to not repeat" or "not to repeat", am I wrong ?
Why ?

Anyway, jam's remarks have already been taken into account except
for the _coalesce_offsets modification.

That's all for this answer whose aim was to quickly give you
feedback at the higher level, I'll take your comments into
account ASAP.

        Vincent



More information about the bazaar mailing list