You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2018/07/19 18:24:40 UTC
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
On Thu, May 31, 2018 at 8:24 AM, <ji...@apache.org> wrote:
> Author: jim
> Date: Thu May 31 13:24:04 2018
> New Revision: 1832609
>
> URL: http://svn.apache.org/viewvc?rev=1832609&view=rev
> Log:
> Merge r1828890, r1832500 from trunk:
>
[...]
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/
> modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=
> 1832609&r1=1832608&r2=1832609&view=diff
> ============================================================
> ==================
> --- httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> Thu May 31 13:24:04 2018
>
[...]
> @@ -70,82 +77,17 @@ static int (*ap_proxy_retry_worker_fn)(c
> * b a d c d a c d b d ...
> *
> */
> -
> static proxy_worker *find_best_byrequests(proxy_balancer *balancer,
> request_rec *r)
> {
> - int i;
> int total_factor = 0;
>
[...]
> + proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer,
> r, is_best_byrequests, &total_factor);
>
This introduced a new hard runtime config ordering problem on
mod_lbmethod_byrequest.so, which must now be loaded AFTER mod_proxy.so.
This was not previously true, as illustrated by mod_lbmethod_heartbeat,
using the ap_proxy_retry_worker using an optional function.
lbmethod sorts before proxy, fwiw.
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 20, 2018 at 2:56 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> So maybe some checking is due there as well if the conditional fetching should be removed?
+1 :)
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/20/2018 02:45 PM, Yann Ylavic wrote:
> On Fri, Jul 20, 2018 at 12:13 PM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> Something like the attached? The mod_lbmethod_byrequests.c part needs to be done for lb modules though.
>
> +/* post_config hook: */
> +static int lbmethod_byrequests_post_config(apr_pool_t *pconf, apr_pool_t *plog,
> + apr_pool_t *ptemp, server_rec *s)
> +{
> +
> + /* lbmethod_byrequests_post_config() will be called twice during
> startup. So, don't
> + * set up the static data the 1st time through. */
> + if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) {
> + return OK;
> + }
> +
> + if (!ap_proxy_balancer_get_best_worker_fn) {
>
> Shouldn't we remove this check to retrieve the function pointer unconditionally?
> With static linking the pointer may not be NULL but still point to
> garbage (mod_lbmethod_byrequests linked statically but mod_proxy
> linked dynamically, if that's ever possible). APR_RETRIEVE_OPTIONAL_FN
> is cheap anyway.
Good point. But I just followed the same pattern as other code in mod_proxy does :-).
So maybe some checking is due there as well if the conditional fetching should be removed?
Regards
Rüdiger
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 20, 2018 at 12:13 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> Something like the attached? The mod_lbmethod_byrequests.c part needs to be done for lb modules though.
+/* post_config hook: */
+static int lbmethod_byrequests_post_config(apr_pool_t *pconf, apr_pool_t *plog,
+ apr_pool_t *ptemp, server_rec *s)
+{
+
+ /* lbmethod_byrequests_post_config() will be called twice during
startup. So, don't
+ * set up the static data the 1st time through. */
+ if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) {
+ return OK;
+ }
+
+ if (!ap_proxy_balancer_get_best_worker_fn) {
Shouldn't we remove this check to retrieve the function pointer unconditionally?
With static linking the pointer may not be NULL but still point to
garbage (mod_lbmethod_byrequests linked statically but mod_proxy
linked dynamically, if that's ever possible). APR_RETRIEVE_OPTIONAL_FN
is cheap anyway.
+ ap_proxy_balancer_get_best_worker_fn =
+ APR_RETRIEVE_OPTIONAL_FN(ap_proxy_balancer_get_best_worker);
+ if (!ap_proxy_balancer_get_best_worker_fn) {
+ ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO()
+ "mod_proxy must be loaded for
mod_lbmethod_byrequests");
+ return !OK;
+ }
+ }
+
+ return OK;
+}
Otherwise, the patch looks good to me, thanks!
Regards,
Yann.
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
Looks like a good direction. From PR62557, the observed modules to be
adjusted, to consume the new opt fn are;
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -423,6 +424,9 @@ SET(mod_http2_extra_sources
modules/http2/h2_task.c modules/http2/h2_util.c
modules/http2/h2_workers.c
)
+SET(mod_lbmethod_bybusyness_extra_libs mod_proxy)
+SET(mod_lbmethod_byrequests_extra_libs mod_proxy)
+SET(mod_lbmethod_bytraffic_extra_libs mod_proxy)
SET(mod_ldap_extra_defines LDAP_DECLARE_EXPORT)
SET(mod_ldap_extra_libs wldap32)
SET(mod_ldap_extra_sources
And this corresponds with
mod_lbmethod_bybusyness.c:
ap_proxy_balancer_get_best_worker(balancer, r, is_best_bybusyness,
mod_lbmethod_byrequests.c: proxy_worker *worker =
ap_proxy_balancer_get_best_worker(balancer, r, is_best_byrequests,
&total_factor);
mod_lbmethod_bytraffic.c: return
ap_proxy_balancer_get_best_worker(balancer, r, is_best_bytraffic,
This patch in 62557 can be ignored once the new optional entry point is
used instead, and these three modules may be loaded again prior to
mod_proxy in httpd.conf.
On Fri, Jul 20, 2018 at 5:13 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 07/19/2018 08:24 PM, William A Rowe Jr wrote:
> > On Thu, May 31, 2018 at 8:24 AM, <jim@apache.org <ma...@apache.org>>
> wrote:
> >
> > Author: jim
> > Date: Thu May 31 13:24:04 2018
> > New Revision: 1832609
> >
> > URL: http://svn.apache.org/viewvc?rev=1832609&view=rev <
> http://svn.apache.org/viewvc?rev=1832609&view=rev>
> > Log:
> > Merge r1828890, r1832500 from trunk:
> >
> > [...]
> >
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/
> modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=
> 1832609&r1=1832608&r2=1832609&view=diff
> > <http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/
> modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=
> 1832609&r1=1832608&r2=1832609&view=diff>
> > ============================================================
> ==================
> > --- httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> (original)
> > +++ httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c
> Thu May 31 13:24:04 2018
> >
> > [...]
> >
> > @@ -70,82 +77,17 @@ static int (*ap_proxy_retry_worker_fn)(c
> > * b a d c d a c d b d ...
> > *
> > */
> > -
> > static proxy_worker *find_best_byrequests(proxy_balancer *balancer,
> > request_rec *r)
> > {
> > - int i;
> > int total_factor = 0;
> >
> > [...]
> >
> > + proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer,
> r, is_best_byrequests, &total_factor);
> >
> >
> > This introduced a new hard runtime config ordering problem on
> mod_lbmethod_byrequest.so, which must now be loaded AFTER
> > mod_proxy.so.
> >
> > This was not previously true, as illustrated by mod_lbmethod_heartbeat,
> using the ap_proxy_retry_worker using an
> > optional function.
> >
> > lbmethod sorts before proxy, fwiw.
> >
>
> Something like the attached? The mod_lbmethod_byrequests.c part needs to
> be done for lb modules though.
>
> Regards
>
> Rüdiger
>
>
>
>
Re: svn commit: r1832609 - in /httpd/httpd/branches/2.4.x: ./
docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/19/2018 08:24 PM, William A Rowe Jr wrote:
> On Thu, May 31, 2018 at 8:24 AM, <jim@apache.org <ma...@apache.org>> wrote:
>
> Author: jim
> Date: Thu May 31 13:24:04 2018
> New Revision: 1832609
>
> URL: http://svn.apache.org/viewvc?rev=1832609&view=rev <http://svn.apache.org/viewvc?rev=1832609&view=rev>
> Log:
> Merge r1828890, r1832500 from trunk:
>
> [...]
>
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=1832609&r1=1832608&r2=1832609&view=diff
> <http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c?rev=1832609&r1=1832608&r2=1832609&view=diff>
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/balancers/mod_lbmethod_byrequests.c Thu May 31 13:24:04 2018
>
> [...]
>
> @@ -70,82 +77,17 @@ static int (*ap_proxy_retry_worker_fn)(c
> * b a d c d a c d b d ...
> *
> */
> -
> static proxy_worker *find_best_byrequests(proxy_balancer *balancer,
> request_rec *r)
> {
> - int i;
> int total_factor = 0;
>
> [...]
>
> + proxy_worker *worker = ap_proxy_balancer_get_best_worker(balancer, r, is_best_byrequests, &total_factor);
>
>
> This introduced a new hard runtime config ordering problem on mod_lbmethod_byrequest.so, which must now be loaded AFTER
> mod_proxy.so.
>
> This was not previously true, as illustrated by mod_lbmethod_heartbeat, using the ap_proxy_retry_worker using an
> optional function.
>
> lbmethod sorts before proxy, fwiw.
>
Something like the attached? The mod_lbmethod_byrequests.c part needs to be done for lb modules though.
Regards
Rüdiger