On 03/15/2011 06:14 AM, Tsantilas Christos wrote:
> So, is it OK to use the 30 seconds as the default dns_timeout value?
Whatever the new default is, please document/commit that change
separately because, unlike the new feature you have implemented, the
default change may have a significant negative effect on some existing
deployments unless they take care to update their configurations.
> If not objection I will commit this patch later today ....
Thank you,
Alex.
> On 03/14/2011 06:35 PM, Alex Rousskov wrote:
>
>>> +static void
>>> +dump_time_msec(StoreEntry * entry, const char *name, uint64_t var)
>>> +{
>>> + storeAppendPrintf(entry, "%s %d seconds and %d milliseconds\n",
>>> name, (int) (var/1000), (int) (var % 1000));
>>> +}
>>
>> I doubt we are consistent with this everywhere, but should we try to
>> dump options in a format compatible with squid.conf syntax? If yes, we
>> could do something like this (at least):
>>
>> if (var % 1000)
>> storeAppendPrintf(entry, "%s %d milliseconds\n", name, (int) var);
>> else
>> storeAppendPrintf(entry, "%s %d seconds\n", name, (int) (var/1000));
>
> OK fixed.
>
>>
>> We can also use T_SECOND_STR and T_MILLISECOND_STR in the above.
> I did not use them because of the missing "s" at the end of these
> strings ("second" not "seconds" etc....)
>
>>
>>> - commSetTimeout(vc->fd, Config.Timeout.idns_query, NULL, NULL);
>>> + const int timeout = (Config.Timeout.idns_query % 1000 ?
>>> + Config.Timeout.idns_query + 1000 :
>>> Config.Timeout.idns_query) / 1000;
>>> +
>>> + commSetTimeout(vc->fd, timeout, NULL, NULL);
>>
>> Perhaps add a source comment explaining that while we are forced to
>> convert to seconds here, the exact timeout will be checked in
>> idnsCheckQueue()?
>> // Comm needs seconds but idnsCheckQueue() will check the exact timeout
>
> done
>
>>
>> Please consider typedef-ing uint64_t as time_msec and using time_msec
>> type as needed. This will help with identifying time-related values in
>> the future.
>
> done
>
> On 03/15/2011 01:25 AM, Amos Jeffries wrote:
>>
>> Yes. We MUST even. People use the cachemgr output these dump routines
>> flow into as copy-n-paste sources for other squid.conf and tutorials.
>>
>>>
>>> if (var % 1000)
>>> storeAppendPrintf(entry, "%s %d milliseconds\n", name, (int) var);
>>> else
>>> storeAppendPrintf(entry, "%s %d seconds\n", name, (int) (var/1000));
>>
>> Please also avoid the 32-bit cast by using %"PRIu64" for all unsigned
>> 64-bit display.
>
> done
>
>>
>> Also, conversion to the largest whole time period (second, minute, hour,
>> day etc as supported by the parser) would be good for user education
>> through the display defaults.
>
> I did not change this one. I do not think it is something important
> because it is unusual to have big time periods, in the cases where the
> user should use milliseconds...
>
>
>
>
