<html><head><style>body{font-family:Helvetica,Arial;font-size:13px}</style></head><body><div style="margin:0px"><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"><div style="margin:0px">Hi,</div><div style="margin:0px"><br></div><div style="margin:0px">Thanks for your detailed review.</div><div style="margin:0px"><br></div><div style="margin:0px"><br></div><div style="margin:0px">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.</div><div style="margin:0px"><br></div><div style="margin:0px">Also I had rewrite the whole commit message trying to be more specific.</div><div style="margin:0px"><br></div><div style="margin:0px">> <span style="color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px">First, why do you change max_connections from 5 to 8? I'm not against,</span></div><span style="color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px">> but any change should have a clear reason. It also worth to mention that</span><br style="box-sizing:border-box;color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px"><span style="color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px">> it possibly was set to 5 by mistake, because the default value of</span><br style="box-sizing:border-box;color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px"><span style="color:rgb(51,51,51);font-family:"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:14px">> another option (CURLOPT_MAXCONNECTS) is 5.</span></div><div style="margin:0px"><font color="#333333" face="Helvetica Neue, Helvetica, Arial, sans-serif"><span style="font-size:14px"><br></span></font><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">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.</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">---</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">index ef0efa4bf..50ff91a36 100644</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">--- a/src/lua/httpc.lua</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">+++ b/src/lua/httpc.lua</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">@@ -41,8 +41,8 @@ local curl_mt</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> --</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> -- Parameters:</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> --</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">--- max_connections - Maximum number of entries in the connection cache */</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">--- max_total_connections - Maximum number of active connections */</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">+-- max_connections - Maximum number of entries in the connection cache</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">+-- max_total_connections - Maximum number of active connections</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> --</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> -- Returns:</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> -- curl object or raise error()</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">@@ -52,8 +52,8 @@ local http_new = function(opts)</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> opts = opts or {}</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">- opts.max_connections = opts.max_connections or 8</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">- opts.max_total_connections = opts.max_total_connections or 8</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">+ opts.max_connections = opts.max_connections or -1</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px">+ opts.max_total_connections = opts.max_total_connections or 0</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> local curl = driver.new(opts.max_connections, opts.max_total_connections)</div><div style="font-family:Helvetica,Arial;font-size:13px;margin:0px"> return setmetatable({ curl = curl, }, curl_mt )</div></div></div> <div class="gmail_signature"></div> <br><p class="airmail_on">On 22 April 2019 at 05:38:19, Alexander Turenko (<a href="mailto:alexander.turenko@tarantool.org">alexander.turenko@tarantool.org</a>) wrote:</p> <blockquote type="cite" class="clean_bq"><span><div><div></div><div>Thanks for the investigation and the patch!
<br>
<br>I propose to polish the commit message to make your results more clear
<br>for anybody who will be interested and my review is mostly about the
<br>wording.
<br>
<br>Re process: It is better to create a branch within tarantool/tarantool
<br>repository named nickname/gh-xxxx-short-description. (It will be easier
<br>to pull your changes or push some fixups upward.)
<br>
<br>Also, please, add my @<a href="http://tarantool.org">tarantool.org</a> email to To or CC when ask for
<br>review; then I'll not miss your email.
<br>
<br>Re code: It is okay, but several things need to be clarified (in the
<br>commit message).
<br>
<br>First, why do you change max_connections from 5 to 8? I'm not against,
<br>but any change should have a clear reason. It also worth to mention that
<br>it possibly was set to 5 by mistake, because the default value of
<br>another option (CURLOPT_MAXCONNECTS) is 5.
<br>
<br>Second. AFAIU, libcurl connections caching is useful when requests are
<br>typically going to the same hosts. So I think we need to state
<br>explicitly that we assume this case before show calculations about
<br>amount of TIME_WAIT sockets and so on.
<br>
<br>It also worth to mention that tarantool's http client configures libcurl
<br>to optimize the case when connections are mainly going to the same hosts
<br>and show the way to configure it for requests for arbitrary hosts: set
<br>max_connections to -1 and max_total_connections to 0 (see lib/multi.c in
<br>curl sources).
<br>
<br>> There are some usecases when current http client may leak sockets (due
<br>> TIME_WAIT timeout). The reason is that the `max_connection` option of
<br>> tarantool's http client (actually is a binding for CURLMOPT_MAXCONNECTS
<br>> option in libcurl) may not be enough for some situations.
<br>
<br>Typo: usecases -> use cases.
<br>
<br>'Leak' is not accurate term. 'opens many new connections and eventually
<br>stuck when exhausted available ports range' -- is this better? However
<br>AFAIR the problem that was arose in the issue in not about TIME_WAIT
<br>sockets. Correct me if I'm wrong, but it seems that we should describe
<br>these two problems separatelly.
<br>
<br>I understand that in this paragraph you try to describe thing 'in
<br>common', but here a reader will have many questions: 'some use cases' --
<br>which? 'may not be enough' -- for what? 'some situations' -- which?
<br>
<br>If you need to describe how curl operates with cached and non-cached
<br>requests with CURLMOPT_MAXCONNECTS and CURLMOPT_MAX_TOTAL_CONNECTIONS
<br>options set to specific values then do it first. When you make this
<br>clear it would be easy to describe which problems are possible and how
<br>they can be handled using max_connections and max_total_connections
<br>options.
<br>
<br>My common suggestions when writing technical texts are try hard to avoid
<br>non-specific descriptions ('some' word is the red flag) and avoid
<br>forward references.
<br>
<br>> This option sets up a size of connection cache, which curl maintains
<br>> during its lifetime. When this cache is full (i.e. all connections are
<br>> waiting for a response from a server), newly created request would
<br>> create a new connection to handle it. In that case right after old
<br>> connection got response, curl has to close that connection to keep up an
<br>> appropriate number of connections in cache. That results in a new socket
<br>> in TIME_WAIT state (for 60 seconds by default).
<br>>
<br>> This is actually easy to calculate. Imagine we have http client with
<br>> default 8 maximum connections in cache. Also lets say we have 16384
<br>> sockets available in the system. We need to make requests to external
<br>> system which responses in 100ms in average. So, if we start making
<br>> request every 11ms (100/9), every finished request will be followed by
<br>> curl closing that socket (switches to TIME_WAIT). Result is about 90
<br>> wasted sockets per second and additional overhead for socket creation
<br>> for each new request.
<br>>
<br>> Also if we do more than 274 requests per second, we will likely to be
<br>> out of available sockets in 1 minute.
<br>>
<br>> The solution is to add another binding for max_total_connections option,
<br>> which will let curl hold new requests, before old ones have finished.
<br>> It also adds more control over sockets resources.
<br>
<br>I don't see this description in the commit message on your branch.
<br>Forgot to update?
<br>
<br>Also we need to add a docbot request (see
<br><a href="https://github.com/tarantool/docbot">https://github.com/tarantool/docbot</a>) and it would be good to move this
<br>description into the request to pass it to the documentation team when
<br>the commit will land to the master.
<br>
<br>>
<br>> -
<br>>
<br>> New opt is ignored if curl version is lower 7.30.0
<br>> Option CURLMOPT_MAX_TOTAL_CONNECTIONS was added from 7.30.0 version
<br>> <a href="https://curl.haxx.se/changes.html#7_30_0">https://curl.haxx.se/changes.html#7_30_0</a>
<br>>
<br>> -
<br>>
<br>> Fixes #3945
<br>>
<br>
<br>
<br>> <a href="https://github.com/RunsFor/tarantool/tree/master">https://github.com/RunsFor/tarantool/tree/master</a>
<br>> <a href="http://github.com/tarantool/tarantool/issues/3945">http://github.com/tarantool/tarantool/issues/3945</a>
<br>
<br>Place a text that in not part of a commit message under '---' mark. It
<br>will allow to apply your commit with `git am` and, more important, don't
<br>confuse a reviewer what is part of a commit message and what is not.
<br>
<br>> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
<br>> index cd44b6054..8566db88d 100644
<br>> --- a/src/lua/httpc.lua
<br>> +++ b/src/lua/httpc.lua
<br>> @@ -41,7 +41,8 @@ local curl_mt
<br>> --
<br>> -- Parameters:
<br>> --
<br>> --- max_connectionss - Maximum number of entries in the connection cache */
<br>> +-- max_connections - Maximum number of entries in the connection cache */
<br>> +-- max_total_connections - Maximum number of active connections */
<br>
<br>'*/' here is just the typo.
<br></div></div></span></blockquote></body></html>