Re: [PATCH] Comm::Connection and friends

On 03/31/2011 07:50 AM, Amos Jeffries wrote:
>> Please s/dataDescriptor/primaryConnection/ or something like that.
>
> Left that unchanged from the existing Server API. name and semantics both.
> It needs co-ordinating with FTP dual-socket requirement (data channel +
> control channel) thrown in for extra confusion against the HTTP which
> would actually be best called "serverConnection".
> "primary" in FTP is control. "data" is where the reply body comes from.
> "primary" in HTTP has no meaning, "data" is where the reply comes from.
> the others do not yet use the server API, but follow the HTTP semantics
> more closely.
>
> I'm happy to change the name here, there is zero increase to the patch
> but only if there is less confusion afterwards.

If FTP calls the control connection "primary", then primaryConnection is
a bad choice. Consider "dataConnection" then to remove the "descriptor"
part that is no longer true, but it is not a big deal to leave as is
until a better term is found.

>> The patch is too long for me to review fully, unfortunately. I
>> understand that portions of it were reviewed earlier, but I cannot
>> easily exclude those portions from the patch so it does not really help.
>
> I can break this down into the per-file patches again along the lines of
> the feature outlined above so its easier to read for audit. The code
> just wont compile if its committed that way.

I do not think breaking into pieces will be of much help at this point
because the total volume will remain the same. Do not waste your time on
that unless others need it.

>> Several changes may have a negative performance effect (e.g., memory
>> pools for persistent connection lists are gone?). There are also many
>> rather complex changes that might change functionality even if we do not
>> expect it. If this patch is committed now, are you going to port it to
>> the v3.2 branch or is it a v3.3-only change?
>
> 3.2.
> Some of those changes (eg forwarding and peer selection) are outstanding
> blockers on 3.2 stability in their own right. The others appear to raise
> performance a little as side effects. Then there are the bugs.

Please document a potential performance regression in a commit message.
I doubt the removal of memory pools will be noticeable here, but we
should get into a habit declaring suspicious spots so that it is easier
to identify them in a big range of revisions during regression testing.

Thank you,

Alex.
Received on Thu Mar 31 2011 - 15:44:50 MDT