httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christophe JAILLET <christophe.jail...@wanadoo.fr>
Subject Re: svn commit: r1696105 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_socache_memcache.xml modules/cache/mod_socache_memcache.c
Date Mon, 24 Aug 2015 06:17:49 GMT
Hi, Yann and thanks for the review.

Le 21/08/2015 15:47, Yann Ylavic a écrit :
> On Sun, Aug 16, 2015 at 12:05 AM,  <jailletc36@apache.org> wrote:
>> Author: jailletc36
>> Date: Sat Aug 15 22:05:08 2015
>> New Revision: 1696105
>>
>> URL: http://svn.apache.org/r1696105
> []
>> Modified: httpd/httpd/trunk/modules/cache/mod_socache_memcache.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_socache_memcache.c?rev=1696105&r1=1696104&r2=1696105&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/cache/mod_socache_memcache.c (original)
>> +++ httpd/httpd/trunk/modules/cache/mod_socache_memcache.c Sat Aug 15 22:05:08 2015
> []
>> @@ -310,6 +319,35 @@ static const ap_socache_provider_t socac
> []
>> +static const char *socache_mc_set_ttl(cmd_parms *cmd, void *dummy,
>> +                                      const char *arg)
>> +{
>> +    socache_mc_svr_cfg *sconf = ap_get_module_config(cmd->server->module_config,
>> +                                                     &socache_memcache_module);
>> +    int i;
>> +
>> +    i = atoi(arg);
>> +
>> +    if ((i < 1) || (i > 1800)) {
> Why a limit of 1800? Maybe the implicit INT_MAX is good enough.
> Also the memcached may never close its connections by itself, or
> always do, so -1 and 0 could also be interesting...
Obviously 0 should be allowed, but I don't see any reason for -1. What 
difference would you make between the 2?

INT_MAX on a 32 bits system is 2147483647. I considered that giving the 
TTL in sec was enough and "more standard". This has to be converted to sec.
So, the maximum allowed value would be 2147. 1800 (half an hour) looked 
a more "logical" value for an end user. That's why I have chosen this limit.

Maybe up to 3600 (an hour) should also be allowed as it is accepted by 
apr_memcache_server_create.

>
>> +        return apr_pstrcat(cmd->pool, cmd->cmd->name,
>> +                           " must be a number between 1 and 1800.", NULL);
>> +    }
>> +
>> +    /* apr_memcache_server_create needs a ttl in usec. */
>> +    sconf->ttl = i * 1000 * 1000;
> sconf->ttl = apr_time_from_sec(i) ?
Didn't do that because of the the different type (apr_time_t (i.e. 
apr_int64_t) vs apr_uint32_t)
But using apr_time_from_sec looks good and self-document the code (i.e. 
ttl in usec)

>
>> +
>> +    return NULL;
>> +}
> Regards,
> Yann.
>



Mime
View raw message