On 11/11/2013 09:11 AM, Amos Jeffries wrote:
> On 7/11/2013 11:27 p.m., Tsantilas Christos wrote:
>> This patch adds the new ACL adaptation_service, to match the name of:
>>
>>    - an adaptation service or group that had been applied to the master
>>      transaction in the past
>>    - an adaptation service or group that is being applied to the master
>>      transaction now
>>
>> An adaptation group is formed by adaptation_service_chain or
>> adaptation_service_set directives. Both REQMOD and RESPMOD services,
>> successful or failed service applications matches this acl.
>>
>> This is a Measurement Factory project
>>
> 
> 1) Can we please get a bit of insight into why/how this is useful addition?

A simple example is when you want log requests based on icap service.
For example use icap_log to log icap requests to a service.


> 
>  The code looks mostly fine, but the text here for proposed commit
> message leaves me wondering why you spent any time coding this up and
> who will ever find it useful.
> 
> 
> 2) code audit ...
> 
> in src/acl/AdaptationService.cc:
> 
> * please use HttpRequest::Pointer instead of HttpRequest* for the local
> variable.
> 
> in src/acl/AdaptationServiceData.cc:
> 
> * please use DBG_CRITICAL instead of '0' for debug level. Also, please
> prefix with "FATAL:" the debug which is followed by self_destruct() to
> highlight the issue.
> 

OK for all of them.


> 
> These do not need another round of review after fixing. +1 on commit
> with the above changes.

OK. I will wait today for more comments and I commit if there are no new
requests.

> 
> Amos
> 
> 

Reply via email to