Re: Client bandwidth limits for squid3-trunk

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. :-)

>> 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.

>>> 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.

>>> 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?

> There is a matching read delay system we will have to match it to
> somehow. We currently have the same queue pattern in three places under
> three names: AcceptLimiter for incoming connection acceptances, read
> "Delays" for delay pools, and now write "Quota" for client pools.

Indeed. However, all three queues are different in nature and are used
in different places so there is no significant code duplication.

Thank you,

Alex.
Received on Thu Oct 14 2010 - 19:20:47 MDT