Re: Client bandwidth limits for squid3-trunk

On 15/10/10 08:20, Alex Rousskov wrote:
> On 10/14/2010 04:19 AM, Amos Jeffries wrote:
>> On 14/10/10 22:32, Tsantilas Christos wrote:
>>> On 10/14/2010 02:35 AM, Amos Jeffries wrote:
>>>> On Wed, 13 Oct 2010 23:09:43 +0300, Tsantilas Christos
>>>> <chtsanti_at_users.sourceforge.net> wrote:
>>>>> Hi all,
>>>>> I am working on adding support for client-side bandwith limiting on
>>>>> squid3. The related description of this feature is at
>>>>> http://wiki.squid-cache.org/Features/ClientBandwidthLimit
>>>>>
>>>>> The feature implemented by Alex Rousskov for his 3p1-rock branch. In
>>>>> this mail I am including a first port for squid-trunk.
>>>>>
>>>>> The patch needs more testing and probably farther development but I am
>>>>> posting the current patch for preview and initial feedback.
>>>>>
>>>>> A short description included in the patch.
>>>>>
>>>>> Regards,
>>>>> Christos
>>>>
>>>> Can't see anything obvious in the code. :)
>
> Do you mean the code is so bad you cannot understand anything or so good
> that you cannot find any problems? I think Christos assumed the former. :-)
>

Sorry Christos. I meant the latter (smiley face = happy with it?).

>
>>> I know :-)
>>> I am still trying to clarify some parts of the code.
>
> It looks like most of the code is well-commented and the patch preamble
> describes the overall architecture. The critical parts are in
> queuing/delaying writes in Comm, without those delays being visible to
> the Comm callers, of course.
>
> I would be happy to help with clarifying the code, but I cannot do that
> without specific questions. IIRC, I did not write the original version,
> but I rewrote most of it, so I should be able to grok what the code does.
>

My only issues are with using a queue that can be replicated for the
read and accept operations.

A week ago I would have needed explanations but I spent a lot of time
last weekend reading the comm_write structures. This all seems to fit
relatively nicely. Albeit sans optimization.

>
>>>> Can you at least make the new functions into members where possible. In
>>>> this case clientdbSetWriteLimiter into ClientInfo::setWriteLimiter.
>
> I am not against that (it is just one function, albeit with conditional
> compilation), but I think we should not do it because it is outside this
> project scope: ClientInfo has no methods now, and all current
> manipulation code is written as functions. We can and should change
> that, of course, but
>
> * _properly_ doing that is not what this project is about and will
> result in many totally irrelevant changes.
>
> * adding just one conditionally-compiled method when everything else is
> done via functions will make the code look less consistent and less clear.
>
> Either way is fine with me though.
>

The other functions are all the scope of my Comm::Io::Write cleanup
patch. This function does not easily enter the writing scope. It's
ClientInfo manipulation.

>
>>>> In hunk @@ -3125,40 +3129,82 @@ it looks like "ClientDelayPools&
>>>> pools(Config.ClientDelay.pools);" could be reduced in scope to inside
>>>> the if().
>
> Yes, it should be.
>
> Also many comments should use Doxygen style. I think this code was
> written before we started encouraging Doxygen use...
>
>
>> depending on Alex we may directly clash between the is
>> and the Comm::Io::Write polishing.
>>
>> I'm unsure how the commHandleWrite function will fit into that layout.
>
> What is Comm::Io::Write polishing layout? Is it the "comm write and
> commfd_table cleanup" patch posted on 2010/10/10?

Yes.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2