You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Nick Kew <ni...@webthing.com> on 2007/06/27 17:08:00 UTC
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
On Wed, 27 Jun 2007 14:17:36 -0000
jfclere@apache.org wrote:
> + * mod_proxy: Arrange the timeout handling.
> + Trunk version of patch:
> + http://svn.apache.org/viewvc?view=rev&revision=550514
> + http://svn.apache.org/viewvc?view=rev&revision=546128
> + +1: jfclere
Looks reasonable, but is there a reference to the problem it solves?
> +
> + * mod_proxy: Improve traces in ap_proxy_http_process_response()
> + to investigate PR37770.
> + Trunk version of patch:
> + http://svn.apache.org/viewvc?view=rev&rev=549420
> + +1: jfclere
Hmmm. This is designed to improve an error message?
+ tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+ rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
+ apr_brigade_destroy(tmp_bb);
Isn't a whole new brigade an unnecessarily overhead (and
potentially large if the function gets used more in future)?
What problem does it solve?
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by jean-frederic clere <jf...@gmail.com>.
Plüm wrote:
>
>> -----Ursprüngliche Nachricht-----
>> Von: jean-frederic clere
>> Gesendet: Donnerstag, 28. Juni 2007 09:11
>> An: dev@httpd.apache.org
>> Betreff: Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
>>
>>
>> Nick Kew wrote:
>>> On Wed, 27 Jun 2007 14:17:36 -0000
>>> jfclere@apache.org wrote:
>>>
>>>> + * mod_proxy: Arrange the timeout handling.
>>>> + Trunk version of patch:
>>>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>>>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>>>> + +1: jfclere
>>> Looks reasonable, but is there a reference to the problem it solves?
>>>
>> It seems there isn't a bug bugzilla yet for this one. should I create
>> one? ;-)
>
> I guess we can relate this to PR11540
Done.
I have now a goal... Review the PR's to see which ones can be closed ;-)
Cheers
Jean-Frederic
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=11540
>
> because the inheritance patches which are already backported only solve a
> part of the problem.
> See also http://issues.apache.org/bugzilla/show_bug.cgi?id=11540#c20
>
> Regards
>
> Rüdiger
>
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: jean-frederic clere
> Gesendet: Donnerstag, 28. Juni 2007 09:11
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
>
>
> Nick Kew wrote:
> > On Wed, 27 Jun 2007 14:17:36 -0000
> > jfclere@apache.org wrote:
> >
> >> + * mod_proxy: Arrange the timeout handling.
> >> + Trunk version of patch:
> >> + http://svn.apache.org/viewvc?view=rev&revision=550514
> >> + http://svn.apache.org/viewvc?view=rev&revision=546128
> >> + +1: jfclere
> >
> > Looks reasonable, but is there a reference to the problem it solves?
> >
>
> It seems there isn't a bug bugzilla yet for this one. should I create
> one? ;-)
I guess we can relate this to PR11540
http://issues.apache.org/bugzilla/show_bug.cgi?id=11540
because the inheritance patches which are already backported only solve a
part of the problem.
See also http://issues.apache.org/bugzilla/show_bug.cgi?id=11540#c20
Regards
Rüdiger
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by jean-frederic clere <jf...@gmail.com>.
Nick Kew wrote:
> On Wed, 27 Jun 2007 14:17:36 -0000
> jfclere@apache.org wrote:
>
>> + * mod_proxy: Arrange the timeout handling.
>> + Trunk version of patch:
>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>> + +1: jfclere
>
> Looks reasonable, but is there a reference to the problem it solves?
>
It seems there isn't a bug bugzilla yet for this one. should I create
one? ;-)
Cheers
Jean-Frederic
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by jean-frederic clere <jf...@gmail.com>.
Ruediger Pluem wrote:
>
> On 06/27/2007 05:51 PM, Jim Jagielski wrote:
>> On Jun 27, 2007, at 11:08 AM, Nick Kew wrote:
>>
>>> On Wed, 27 Jun 2007 14:17:36 -0000
>>> jfclere@apache.org wrote:
>>>
>>>> + * mod_proxy: Arrange the timeout handling.
>>>> + Trunk version of patch:
>>>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>>>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>>>> + +1: jfclere
>>> Looks reasonable, but is there a reference to the problem it solves?
>>>
>>>> +
>>>> + * mod_proxy: Improve traces in ap_proxy_http_process_response()
>>>> + to investigate PR37770.
>>>> + Trunk version of patch:
>>>> + http://svn.apache.org/viewvc?view=rev&rev=549420
>>>> + +1: jfclere
>>> Hmmm. This is designed to improve an error message?
>>>
>>> + tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
>>> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
>>> + apr_brigade_destroy(tmp_bb);
>>>
>>> Isn't a whole new brigade an unnecessarily overhead (and
>>> potentially large if the function gets used more in future)?
>>> What problem does it solve?
>>>
>> Yeah... all this just so we can see the return val
>> of ap_rgetline()??
>
> Yes, but have a look at ap_getline in protocol.c which was used before instead
> of ap_proxygetline. It does exactly the same thing regarding the brigade.
> So his solution is not less efficient than the old one. Of course it can be discussed
> whether the old one was efficient enough :-).
> I guess we can create a brigade for this purpose at the beginning of
> ap_proxy_http_process_response pass it to ap_proxygetline and do a apr_brigade_cleanup each time
> after we have called ap_proxygetline. But in this case we can also use
> ap_rgetline directly in ap_proxy_http_process_response
I like the idea.
Cheers
Jean-Frederic
> as there is not much left of
> ap_proxygetline in this case.
> The other aspect of this patch is the matter of code duplication. The question is
> whether it is good to nearly duplicate the code of ap_getline and thus have to manage
> it twice in different locations.
>
>
> Regards
>
> Ru"diger
>
>
>
>
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 27, 2007, at 12:20 PM, Ruediger Pluem wrote:
>
>
> On 06/27/2007 05:51 PM, Jim Jagielski wrote:
>>
>> On Jun 27, 2007, at 11:08 AM, Nick Kew wrote:
>>
>>> On Wed, 27 Jun 2007 14:17:36 -0000
>>> jfclere@apache.org wrote:
>>>
>>>> + * mod_proxy: Arrange the timeout handling.
>>>> + Trunk version of patch:
>>>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>>>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>>>> + +1: jfclere
>>>
>>> Looks reasonable, but is there a reference to the problem it solves?
>>>
>>>> +
>>>> + * mod_proxy: Improve traces in
>>>> ap_proxy_http_process_response()
>>>> + to investigate PR37770.
>>>> + Trunk version of patch:
>>>> + http://svn.apache.org/viewvc?view=rev&rev=549420
>>>> + +1: jfclere
>>>
>>> Hmmm. This is designed to improve an error message?
>>>
>>> + tmp_bb = apr_brigade_create(r->pool, r->connection-
>>> >bucket_alloc);
>>> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
>>> + apr_brigade_destroy(tmp_bb);
>>>
>>> Isn't a whole new brigade an unnecessarily overhead (and
>>> potentially large if the function gets used more in future)?
>>> What problem does it solve?
>>>
>>
>> Yeah... all this just so we can see the return val
>> of ap_rgetline()??
>
> Yes, but have a look at ap_getline in protocol.c which was used
> before instead
> of ap_proxygetline. It does exactly the same thing regarding the
> brigade.
I wasn't concerned about the brigade, but rather the extra
layer of complexity just so we see a return value...
Is it worth it?
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Ruediger Pluem <rp...@apache.org>.
On 06/27/2007 05:51 PM, Jim Jagielski wrote:
>
> On Jun 27, 2007, at 11:08 AM, Nick Kew wrote:
>
>> On Wed, 27 Jun 2007 14:17:36 -0000
>> jfclere@apache.org wrote:
>>
>>> + * mod_proxy: Arrange the timeout handling.
>>> + Trunk version of patch:
>>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>>> + +1: jfclere
>>
>> Looks reasonable, but is there a reference to the problem it solves?
>>
>>> +
>>> + * mod_proxy: Improve traces in ap_proxy_http_process_response()
>>> + to investigate PR37770.
>>> + Trunk version of patch:
>>> + http://svn.apache.org/viewvc?view=rev&rev=549420
>>> + +1: jfclere
>>
>> Hmmm. This is designed to improve an error message?
>>
>> + tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
>> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
>> + apr_brigade_destroy(tmp_bb);
>>
>> Isn't a whole new brigade an unnecessarily overhead (and
>> potentially large if the function gets used more in future)?
>> What problem does it solve?
>>
>
> Yeah... all this just so we can see the return val
> of ap_rgetline()??
Yes, but have a look at ap_getline in protocol.c which was used before instead
of ap_proxygetline. It does exactly the same thing regarding the brigade.
So his solution is not less efficient than the old one. Of course it can be discussed
whether the old one was efficient enough :-).
I guess we can create a brigade for this purpose at the beginning of
ap_proxy_http_process_response pass it to ap_proxygetline and do a apr_brigade_cleanup each time
after we have called ap_proxygetline. But in this case we can also use
ap_rgetline directly in ap_proxy_http_process_response as there is not much left of
ap_proxygetline in this case.
The other aspect of this patch is the matter of code duplication. The question is
whether it is good to nearly duplicate the code of ap_getline and thus have to manage
it twice in different locations.
Regards
Ru"diger
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by jean-frederic clere <jf...@gmail.com>.
Jim Jagielski wrote:
>
> On Jun 27, 2007, at 11:08 AM, Nick Kew wrote:
>
>> On Wed, 27 Jun 2007 14:17:36 -0000
>> jfclere@apache.org wrote:
>>
>>> + * mod_proxy: Arrange the timeout handling.
>>> + Trunk version of patch:
>>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>>> + +1: jfclere
>>
>> Looks reasonable, but is there a reference to the problem it solves?
>>
>>> +
>>> + * mod_proxy: Improve traces in ap_proxy_http_process_response()
>>> + to investigate PR37770.
>>> + Trunk version of patch:
>>> + http://svn.apache.org/viewvc?view=rev&rev=549420
>>> + +1: jfclere
>>
>> Hmmm. This is designed to improve an error message?
>>
>> + tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
>> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
>> + apr_brigade_destroy(tmp_bb);
>>
>> Isn't a whole new brigade an unnecessarily overhead (and
>> potentially large if the function gets used more in future)?
>> What problem does it solve?
>>
>
> Yeah... all this just so we can see the return val
> of ap_rgetline()??
>
>
Yes. When a back-end server failed we need to know what was wrong. The
actual code gives
no clues of what happends.
Cheers
Jean-Frederic
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 27, 2007, at 11:08 AM, Nick Kew wrote:
> On Wed, 27 Jun 2007 14:17:36 -0000
> jfclere@apache.org wrote:
>
>> + * mod_proxy: Arrange the timeout handling.
>> + Trunk version of patch:
>> + http://svn.apache.org/viewvc?view=rev&revision=550514
>> + http://svn.apache.org/viewvc?view=rev&revision=546128
>> + +1: jfclere
>
> Looks reasonable, but is there a reference to the problem it solves?
>
>> +
>> + * mod_proxy: Improve traces in ap_proxy_http_process_response()
>> + to investigate PR37770.
>> + Trunk version of patch:
>> + http://svn.apache.org/viewvc?view=rev&rev=549420
>> + +1: jfclere
>
> Hmmm. This is designed to improve an error message?
>
> + tmp_bb = apr_brigade_create(r->pool, r->connection-
> >bucket_alloc);
> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
> + apr_brigade_destroy(tmp_bb);
>
> Isn't a whole new brigade an unnecessarily overhead (and
> potentially large if the function gets used more in future)?
> What problem does it solve?
>
Yeah... all this just so we can see the return val
of ap_rgetline()??