Re: [PATCH] ICP and HTCP and StoreID

On 2014-03-12 12:12, Alex Rousskov wrote:
> The patch pasted below is related to a long discussion on squid-users.
> See the "ICP and HTCP and StoreID" thread there.
>
> On 02/14/2014 04:38 AM, Nikolai Gorchilov wrote:
>> On Fri, Feb 14, 2014 at 7:22 AM, Alex Rousskov wrote:
>>> On 02/13/2014 06:05 PM, Nikolai Gorchilov wrote:
>>>> Current implementation prevents the refresh.
>
>>> We know that the current implementation is broken, no questions about
>>> it! IIRC, the developer responsible for that breakage promised to fix
>>> it
>>> when the code with a known breakage was committed, but even if he
>>> does
>>> not, Amos, I, or others will [eventually].
>
>> I got the attention of one of my team mates regarding this.
>> Unfortunately he didn't have enough time to comprehend the complete
>> Squid logic, thus the ugliness of his patch. If you provide me with
>> high-level design advise for a proper fix, we may try to produce one.
>
> Some time passed since the above exchange, and Niki has tested the
> following patch. It helped when Squid was forwarding a request to a
> peer
> after a UDP_HIT for a StoreID-mapped object.
>
>
>> --- src/http.cc 2014-03-07 15:40:37.945321671 +0200
>> +++ src/http.cc 2014-03-07 15:41:36.853321533 +0200
>> @@ -2106,7 +2106,7 @@
>> Http::ProtocolVersion httpver(1,1);
>> const char * url;
>> if (_peer && !_peer->options.originserver)
>> - url = entry->url();
>> + url = urlCanonical(request);
>> else
>> url = request->urlpath.termedBuf();
>> mb->Printf("%s %s %s/%d.%d\r\n",
>
>
> The patch makes sense to me: StoreEntry::url() may have nothing to do
> with the URL, just like the old MemObject::url() which is already
> renamed to MemObject::storeId(). The patch does not fix all the
> problems
> discussed on squid-users, but it also does not make any of them worse
> AFAICT.
>
> I have not tested the above patch, have not considered alternatives,
> and
> do not want to become responsible for this Store ID bug or its fix at
> this time.
>
>
> If there are no objections or volunteers to properly introduce this
> change, I suggest committing the above fix "as is".
>

+1. Looks right to me too.

Amos
Received on Wed Mar 12 2014 - 00:11:14 MDT