On 03/05/12 13:08, Tim Foster wrote:
On 03/ 6/12 05:50 AM, Shawn Walker wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/poisoned-cache/poisoned-cache-webrev/
Thanks for taking a look,
src/modules/client/transport/repo.py:
line 1034: it should set header["Pragma"] = "no-cache" as well;
that's also true for line 513; necessary for HTTP 1.0 caches [1]
Good point - I've fixed both of those.
lines 213, 1037, 1752: need extra newline
For 213, it's the same class - other places where we switch from
defining normal vs. static methods don't seem to do that. I've fixed the
others though.
lines 1750, 2019: I'd make these docstrings the same and something
like: """Pointless to attempt refetch of corrupt content for
this protocol."""
Sounds good.
src/modules/client/transport/transport.py:
line 917: this comment is subtly different for each retry case
I've fixed that.
lines 918-926: so the problems I see with this approach are (and it
applies to most of the other cases):
* it relies on the number of retries left being sufficient
to retrieve the content (i.e. retry_count) that's probably ok,
but there should be a comment
Yes, I realised that at the time, and sort of threw my hands up in the
air at what to do in the face of a several failed attempts, followed by
one attempt which passes corrupt content... I've added comments.
* if this is the last retry already, then the failure
won't be added to failedreqs / failures (oops), a simple
fix for that would be to track the number of retries and
skip the redownload if retries == retry_count
Good point - thanks. Yes that seems reasonable.
...
I've got an incremental, and new webrev at:
https://cr.opensolaris.org/action/browse/pkg/timf/poisoned-cache-v2/
So, I'm sorry for not realising this before, but each loop through is
not another retry since there may be multiple origins involved.
The cleanest way to resolve this (but it will cause a lot more churn) is
to have __gen_repo yield 'i' as part of the tuple it returns so you know
the number of the attempt.
The only other way I can think of doing it is something like this:
first_repo = None
retries = 0
for d, v in self.__gen_repo(...):
if not last_repo:
last_repo = d
retries += 1
elif first_repo and first_repo == d:
retries += 1
...as I said, cleaner way is to have __gen_repo() yield the attempt number.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
