Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Jim Jagielski wrote: On Jun 27, 2007, at 11:08 AM, Nick Kew wrote: On Wed, 27 Jun 2007 14:17:36 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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
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 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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 Rudiger
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
Nick Kew wrote: On Wed, 27 Jun 2007 14:17:36 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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
-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 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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
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 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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
On Wed, 27 Jun 2007 14:17:36 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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
On Jun 27, 2007, at 11:08 AM, Nick Kew wrote: On Wed, 27 Jun 2007 14:17:36 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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()??
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
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 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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 Rudiger
Re: svn commit: r551171 - /httpd/httpd/branches/2.2.x/STATUS
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 - [EMAIL PROTECTED] wrote: +* mod_proxy: Arrange the timeout handling. + Trunk version of patch: +http://svn.apache.org/viewvc?view=revrevision=550514 +http://svn.apache.org/viewvc?view=revrevision=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=revrev=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?
