[tarantool-patches] Re: [PATCH] Pass max_total_connections parameter to httpclient

["Илья Конюхов" <runsfor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7244 bytes --]

Hi,

Thanks for your detailed review.


I added couple simple changes to the patch (see diff from original patch
below) and pushed it to the tarantool/tarantool repo. See
branch runsfor/gh-3945-pass-new-opt-to-httpc.

Also I had rewrite the whole commit message trying to be more specific.

> First, why do you change max_connections from 5 to 8? I'm not against,
> but any change should have a clear reason. It also worth to mention that
> it possibly was set to 5 by mistake, because the default value of
> another option (CURLOPT_MAXCONNECTS) is 5.

I agree with your comments about options defaults, so I changed defaults to
match documentation. I was not able to find any reasons why max_connections
option was default to 5. It was originally added in tarantool/curl repo
without any comments and then migrate into tarantool.

---
diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
index ef0efa4bf..50ff91a36 100644
--- a/src/lua/httpc.lua
+++ b/src/lua/httpc.lua
@@ -41,8 +41,8 @@ local curl_mt
 --
 --  Parameters:
 --
---  max_connections -  Maximum number of entries in the connection cache */
---  max_total_connections -  Maximum number of active connections */
+--  max_connections -  Maximum number of entries in the connection cache
+--  max_total_connections -  Maximum number of active connections
 --
 --  Returns:
 --  curl object or raise error()
@@ -52,8 +52,8 @@ local http_new = function(opts)

     opts = opts or {}

-    opts.max_connections = opts.max_connections or 8
-    opts.max_total_connections = opts.max_total_connections or 8
+    opts.max_connections = opts.max_connections or -1
+    opts.max_total_connections = opts.max_total_connections or 0

     local curl = driver.new(opts.max_connections,
opts.max_total_connections)
     return setmetatable({ curl = curl, }, curl_mt )

On 22 April 2019 at 05:38:19, Alexander Turenko (
alexander.turenko@tarantool.org) wrote:

Thanks for the investigation and the patch!

I propose to polish the commit message to make your results more clear
for anybody who will be interested and my review is mostly about the
wording.

Re process: It is better to create a branch within tarantool/tarantool
repository named nickname/gh-xxxx-short-description. (It will be easier
to pull your changes or push some fixups upward.)

Also, please, add my @tarantool.org email to To or CC when ask for
review; then I'll not miss your email.

Re code: It is okay, but several things need to be clarified (in the
commit message).

First, why do you change max_connections from 5 to 8? I'm not against,
but any change should have a clear reason. It also worth to mention that
it possibly was set to 5 by mistake, because the default value of
another option (CURLOPT_MAXCONNECTS) is 5.

Second. AFAIU, libcurl connections caching is useful when requests are
typically going to the same hosts. So I think we need to state
explicitly that we assume this case before show calculations about
amount of TIME_WAIT sockets and so on.

It also worth to mention that tarantool's http client configures libcurl
to optimize the case when connections are mainly going to the same hosts
and show the way to configure it for requests for arbitrary hosts: set
max_connections to -1 and max_total_connections to 0 (see lib/multi.c in
curl sources).

> There are some usecases when current http client may leak sockets (due
> TIME_WAIT timeout). The reason is that the `max_connection` option of
> tarantool's http client (actually is a binding for CURLMOPT_MAXCONNECTS
> option in libcurl) may not be enough for some situations.

Typo: usecases -> use cases.

'Leak' is not accurate term. 'opens many new connections and eventually
stuck when exhausted available ports range' -- is this better? However
AFAIR the problem that was arose in the issue in not about TIME_WAIT
sockets. Correct me if I'm wrong, but it seems that we should describe
these two problems separatelly.

I understand that in this paragraph you try to describe thing 'in
common', but here a reader will have many questions: 'some use cases' --
which? 'may not be enough' -- for what? 'some situations' -- which?

If you need to describe how curl operates with cached and non-cached
requests with CURLMOPT_MAXCONNECTS and CURLMOPT_MAX_TOTAL_CONNECTIONS
options set to specific values then do it first. When you make this
clear it would be easy to describe which problems are possible and how
they can be handled using max_connections and max_total_connections
options.

My common suggestions when writing technical texts are try hard to avoid
non-specific descriptions ('some' word is the red flag) and avoid
forward references.

> This option sets up a size of connection cache, which curl maintains
> during its lifetime. When this cache is full (i.e. all connections are
> waiting for a response from a server), newly created request would
> create a new connection to handle it. In that case right after old
> connection got response, curl has to close that connection to keep up an
> appropriate number of connections in cache. That results in a new socket
> in TIME_WAIT state (for 60 seconds by default).
>
> This is actually easy to calculate. Imagine we have http client with
> default 8 maximum connections in cache. Also lets say we have 16384
> sockets available in the system. We need to make requests to external
> system which responses in 100ms in average. So, if we start making
> request every 11ms (100/9), every finished request will be followed by
> curl closing that socket (switches to TIME_WAIT). Result is about 90
> wasted sockets per second and additional overhead for socket creation
> for each new request.
>
> Also if we do more than 274 requests per second, we will likely to be
> out of available sockets in 1 minute.
>
> The solution is to add another binding for max_total_connections option,
> which will let curl hold new requests, before old ones have finished.
> It also adds more control over sockets resources.

I don't see this description in the commit message on your branch.
Forgot to update?

Also we need to add a docbot request (see
https://github.com/tarantool/docbot) and it would be good to move this
description into the request to pass it to the documentation team when
the commit will land to the master.

>
> -
>
> New opt is ignored if curl version is lower 7.30.0
> Option CURLMOPT_MAX_TOTAL_CONNECTIONS was added from 7.30.0 version
> https://curl.haxx.se/changes.html#7_30_0
>
> -
>
> Fixes #3945
>


> https://github.com/RunsFor/tarantool/tree/master
> http://github.com/tarantool/tarantool/issues/3945

Place a text that in not part of a commit message under '---' mark. It
will allow to apply your commit with `git am` and, more important, don't
confuse a reviewer what is part of a commit message and what is not.

> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index cd44b6054..8566db88d 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -41,7 +41,8 @@ local curl_mt
> --
> -- Parameters:
> --
> --- max_connectionss - Maximum number of entries in the connection cache
*/
> +-- max_connections - Maximum number of entries in the connection cache
*/
> +-- max_total_connections - Maximum number of active connections */

'*/' here is just the typo.

[-- Attachment #2: Type: text/html, Size: 12255 bytes --]