[squid-dev] [PATCH] Honor peer timeouts when forwarding CONNECTs

[Eduard Bagdasaryan]

Just a reminder: are there any more suggestions/remarks
before this feature can be applied?


Eduard.


On 26.03.2017 00:04, Eduard Bagdasaryan wrote:
> Adjusted the patch accordingly.
>
>
> Eduard.
>
>
> On 22.03.2017 15:58, Amos Jeffries wrote:
>> On 17/03/2017 2:11 a.m., Eduard Bagdasaryan wrote:
>>> On 16.03.2017 10:15, Amos Jeffries wrote:
>>>
>>>> in src/neighbours.cc:
>>>>
>>>> * peerConnectTimeout() should be a member of the CachePeer class yes?
>>> The initial patch version did as you suggest, but then we decided
>>> to use separate method instead, to avoid 'heavy' dependency on
>>> SquidConfig.h (which periodically causes linkage problems, IIRC).
>>>
>> Hmm. Okay for now, but please add a TODO or XXX about that to the 
>> function.
>>
>>>> in src/tunnel.cc:
>>>>
>>>> * "start" is an action name and we use it (almost?) exclusively for 
>>>> Job
>>>> initiation. By comparison "started" means/implies more clearly a state
>>>> or time point. The Tunnel member stores a time point.
>>>>   - IMO both are bad, but "started" is better than "start". Better 
>>>> still
>>>> would be a name that actually describes *what* has been start(ed).
>>> I do not mind renaming it, e.g., into 'creationTime'.
>>>
>> Sounds good to me.
>>
>>>> in src/PeerPoolMgr.cc:
>>>>
>>>> * in PeerPoolMgr::handleOpenedConnection() I see a line doing max(1,
>>>> (peerTimeout - timeUsed)); just like the code in FwdState.cc
>>>> FwdState::connectDone().
>>>>   BUT, this PoolMgr is (wrongly) using int instead of time_t. Perhapse
>>>> that should be changed and something de-duplicate that max(1, x) 
>>>> usage?
>>> I am not against changing int to time_t there. As for 'max', how about
>>> creating inside neighbors.cc a helper method:
>>> time_t PositiveTimeout(const time_t timeout) { return max(1, 
>>> timeout); }
>>>
>> Sounds good. I did a few tests to see if we could also avoid the
>> static_cast, but it seems not in any simple way. So dont forget that in
>> the new function.
>>
>>
>>>> in src/comm/Connection.cc:
>>>>
>>>> * I dont see why EnoughTimeToReForward() should be in the Comm::
>>>> namespace. It is about messageing layer timing. So is more appropriate
>>>> to be in FwdState:: or maybe SquidTime.h/time.cc.
>>>>   - if you pick time.cc that would also mean the fairly generic time_t
>>>> static functions it calls would move to time.cc where they seem more
>>>> appropriate.
>>> Again, I am not against moving this method and ForwardTimeout() into
>>> FwdState.cc (keeping related '*Forward' methods there makes some 
>>> sense).
>>>
>> Okay, I can live with that.
>>
>> Amos
>>
>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170412/3dbfcf81/attachment.html>