You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2017/10/24 13:28:42 UTC
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
I don't understand this patch. It looks like we are no lingering externally
representing ldfactor as a float (e.g.: 2.50). Is that right? If so,
I'm not sure why...
> On Oct 24, 2017, at 6:50 AM, ylavic@apache.org wrote:
>
> Author: ylavic
> Date: Tue Oct 24 10:50:34 2017
> New Revision: 1813167
>
> URL: http://svn.apache.org/viewvc?rev=1813167&view=rev
> Log:
> mod_proxy_balancer: fix runtime lbfactor value changed in 2.4.28.
>
> It is assumed to be between 1 and 100 by lbmethods, so normalize it
> accordingly.
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/proxy/mod_proxy.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1813167&r1=1813166&r2=1813167&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Tue Oct 24 10:50:34 2017
> @@ -1,6 +1,10 @@
> -*- coding: utf-8 -*-
> Changes with Apache 2.5.0
>
> + *) mod_proxy_balancer: fix runtime lbfactor value changed in 2.4.28. It is
> + assumed to be between 1 and 100 by lbmethods, so normalize it accordingly.
> + [Yann Ylavic]
> +
> *) mod_md: v1.0.1, ServerName/Alias names from pure-http: virtual hosts are no longer
> auto-added to a Managed Domain. Error counts of jobs are presisted. When the server
> restarts (gracefully) any errored staging areas are purged to reset the signup/renewal
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.c?rev=1813167&r1=1813166&r2=1813167&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.c Tue Oct 24 10:50:34 2017
> @@ -104,13 +104,13 @@ static const char *set_worker_param(apr_
>
> if (!strcasecmp(key, "loadfactor")) {
> /* Normalized load factor. Used with BalancerMember,
> - * it is a number between 1 and 100.
> + * it is a number between 1 and 100 (or 0.01 and 1.0).
> */
> double fval = atof(val);
> ival = fval * 100.0;
> if (ival < 100 || ival > 10000)
> return "LoadFactor must be a number between 1..100";
> - worker->s->lbfactor = ival;
> + worker->s->lbfactor = ival / 100;
> }
> else if (!strcasecmp(key, "retry")) {
> /* If set it will give the retry timeout for the worker
> @@ -2883,7 +2883,7 @@ static int proxy_status_hook(request_rec
> ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, *worker), NULL);
> ap_rvputs(r, "</td><td>", (*worker)->s->route, NULL);
> ap_rvputs(r, "</td><td>", (*worker)->s->redirect, NULL);
> - ap_rprintf(r, "</td><td>%.2f</td>", (float)((*worker)->s->lbfactor)/100.0);
> + ap_rprintf(r, "</td><td>%d</td>", (*worker)->s->lbfactor);
> ap_rprintf(r, "<td>%d</td>", (*worker)->s->lbset);
> ap_rprintf(r, "<td>%" APR_SIZE_T_FMT "</td><td>",
> (*worker)->s->elected);
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=1813167&r1=1813166&r2=1813167&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Tue Oct 24 10:50:34 2017
> @@ -1093,11 +1093,10 @@ static int balancer_handler(request_rec
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01192) "settings worker params");
>
> if ((val = apr_table_get(params, "w_lf"))) {
> - int ival;
> double fval = atof(val);
> - ival = fval * 100.0;
> + int ival = fval * 100.0;
> if (ival >= 100 && ival <= 10000) {
> - wsel->s->lbfactor = ival;
> + wsel->s->lbfactor = ival / 100;
> if (bsel)
> recalc_factors(bsel);
> }
> @@ -1364,8 +1363,8 @@ static int balancer_handler(request_rec
> "</httpd:scheme>\n", NULL);
> ap_rvputs(r, " <httpd:hostname>", worker->s->hostname,
> "</httpd:hostname>\n", NULL);
> - ap_rprintf(r, " <httpd:loadfactor>%.2f</httpd:loadfactor>\n",
> - (float)(worker->s->lbfactor)/100.0);
> + ap_rprintf(r, " <httpd:loadfactor>%d</httpd:loadfactor>\n",
> + worker->s->lbfactor);
> ap_rprintf(r,
> " <httpd:port>%d</httpd:port>\n",
> worker->s->port);
> @@ -1418,8 +1417,8 @@ static int balancer_handler(request_rec
> " <httpd:lbstatus>%d</httpd:lbstatus>\n",
> worker->s->lbstatus);
> ap_rprintf(r,
> - " <httpd:loadfactor>%.2f</httpd:loadfactor>\n",
> - (float)(worker->s->lbfactor)/100.0);
> + " <httpd:loadfactor>%d</httpd:loadfactor>\n",
> + worker->s->lbfactor);
> ap_rprintf(r,
> " <httpd:transferred>%" APR_OFF_T_FMT "</httpd:transferred>\n",
> worker->s->transferred);
> @@ -1605,7 +1604,7 @@ static int balancer_handler(request_rec
> NULL);
> ap_rvputs(r, "</td><td>",
> ap_escape_html(r->pool, worker->s->redirect), NULL);
> - ap_rprintf(r, "</td><td>%.2f</td>", (float)(worker->s->lbfactor)/100.0);
> + ap_rprintf(r, "</td><td>%d</td>", worker->s->lbfactor);
> ap_rprintf(r, "<td>%d</td><td>", worker->s->lbset);
> ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, worker), NULL);
> ap_rputs("</td>", r);
> @@ -1640,7 +1639,7 @@ static int balancer_handler(request_rec
> ap_rputs("<form method='POST' enctype='application/x-www-form-urlencoded' action='", r);
> ap_rvputs(r, ap_escape_uri(r->pool, action), "'>\n", NULL);
> ap_rputs("<table><tr><td>Load factor:</td><td><input name='w_lf' id='w_lf' type=text ", r);
> - ap_rprintf(r, "value='%.2f'></td></tr>\n", (float)(wsel->s->lbfactor)/100.0);
> + ap_rprintf(r, "value='%d'></td></tr>\n", wsel->s->lbfactor);
> ap_rputs("<tr><td>LB Set:</td><td><input name='w_ls' id='w_ls' type=text ", r);
> ap_rprintf(r, "value='%d'></td></tr>\n", wsel->s->lbset);
> ap_rputs("<tr><td>Route:</td><td><input name='w_wr' id='w_wr' type=text ", r);
>
>
AW: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
Same here: I don't see why the lbmethods rely on it between 1 and 100 and why they cannot work with 100 till 10000
Effectively we kill values like 2.5 and the new comment
> it is a number between 1 and 100 (or 0.01 and 1.0).
is wrong since 0.01 is not an allowed value (it will fail the check below).
Regards
Rüdiger
> -----Ursprüngliche Nachricht-----
> Von: Jim Jagielski [mailto:jim@jaguNET.com]
> Gesendet: Dienstag, 24. Oktober 2017 15:29
> An: dev@httpd.apache.org
> Cc: cvs@httpd.apache.org
> Betreff: Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
> modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
>
> I don't understand this patch. It looks like we are no lingering
> externally
> representing ldfactor as a float (e.g.: 2.50). Is that right? If so,
> I'm not sure why...
>
> > On Oct 24, 2017, at 6:50 AM, ylavic@apache.org wrote:
> >
> > Author: ylavic
> > Date: Tue Oct 24 10:50:34 2017
> > New Revision: 1813167
> >
> > URL: http://svn.apache.org/viewvc?rev=1813167&view=rev
> > Log:
> > mod_proxy_balancer: fix runtime lbfactor value changed in 2.4.28.
> >
> > It is assumed to be between 1 and 100 by lbmethods, so normalize it
> > accordingly.
> >
> >
> > Modified:
> > httpd/httpd/trunk/CHANGES
> > httpd/httpd/trunk/modules/proxy/mod_proxy.c
> > httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> >
> > Modified: httpd/httpd/trunk/CHANGES
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1813167&r1=18
> 13166&r2=1813167&view=diff
> >
> ========================================================================
> ======
> > --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> > +++ httpd/httpd/trunk/CHANGES [utf-8] Tue Oct 24 10:50:34 2017
> > @@ -1,6 +1,10 @@
> > -*- coding:
> utf-8 -*-
> > Changes with Apache 2.5.0
> >
> > + *) mod_proxy_balancer: fix runtime lbfactor value changed in
> 2.4.28. It is
> > + assumed to be between 1 and 100 by lbmethods, so normalize it
> accordingly.
> > + [Yann Ylavic]
> > +
> > *) mod_md: v1.0.1, ServerName/Alias names from pure-http: virtual
> hosts are no longer
> > auto-added to a Managed Domain. Error counts of jobs are
> presisted. When the server
> > restarts (gracefully) any errored staging areas are purged to
> reset the signup/renewal
> >
> > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.c
> ?rev=1813167&r1=1813166&r2=1813167&view=diff
> >
> ========================================================================
> ======
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy.c Tue Oct 24 10:50:34
> 2017
> > @@ -104,13 +104,13 @@ static const char *set_worker_param(apr_
> >
> > if (!strcasecmp(key, "loadfactor")) {
> > /* Normalized load factor. Used with BalancerMember,
> > - * it is a number between 1 and 100.
> > + * it is a number between 1 and 100 (or 0.01 and 1.0).
> > */
> > double fval = atof(val);
> > ival = fval * 100.0;
> > if (ival < 100 || ival > 10000)
> > return "LoadFactor must be a number between 1..100";
> > - worker->s->lbfactor = ival;
> > + worker->s->lbfactor = ival / 100;
> > }
> > else if (!strcasecmp(key, "retry")) {
> > /* If set it will give the retry timeout for the worker
> > @@ -2883,7 +2883,7 @@ static int proxy_status_hook(request_rec
> > ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, *worker),
> NULL);
> > ap_rvputs(r, "</td><td>", (*worker)->s->route, NULL);
> > ap_rvputs(r, "</td><td>", (*worker)->s->redirect,
> NULL);
> > - ap_rprintf(r, "</td><td>%.2f</td>",
> (float)((*worker)->s->lbfactor)/100.0);
> > + ap_rprintf(r, "</td><td>%d</td>", (*worker)->s-
> >lbfactor);
> > ap_rprintf(r, "<td>%d</td>", (*worker)->s->lbset);
> > ap_rprintf(r, "<td>%" APR_SIZE_T_FMT "</td><td>",
> > (*worker)->s->elected);
> >
> > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_b
> alancer.c?rev=1813167&r1=1813166&r2=1813167&view=diff
> >
> ========================================================================
> ======
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Tue Oct 24
> 10:50:34 2017
> > @@ -1093,11 +1093,10 @@ static int balancer_handler(request_rec
> > ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01192)
> "settings worker params");
> >
> > if ((val = apr_table_get(params, "w_lf"))) {
> > - int ival;
> > double fval = atof(val);
> > - ival = fval * 100.0;
> > + int ival = fval * 100.0;
> > if (ival >= 100 && ival <= 10000) {
> > - wsel->s->lbfactor = ival;
> > + wsel->s->lbfactor = ival / 100;
> > if (bsel)
> > recalc_factors(bsel);
> > }
> > @@ -1364,8 +1363,8 @@ static int balancer_handler(request_rec
> > "</httpd:scheme>\n", NULL);
> > ap_rvputs(r, " <httpd:hostname>", worker->s-
> >hostname,
> > "</httpd:hostname>\n", NULL);
> > - ap_rprintf(r, "
> <httpd:loadfactor>%.2f</httpd:loadfactor>\n",
> > - (float)(worker->s->lbfactor)/100.0);
> > + ap_rprintf(r, "
> <httpd:loadfactor>%d</httpd:loadfactor>\n",
> > + worker->s->lbfactor);
> > ap_rprintf(r,
> > " <httpd:port>%d</httpd:port>\n",
> > worker->s->port);
> > @@ -1418,8 +1417,8 @@ static int balancer_handler(request_rec
> > "
> <httpd:lbstatus>%d</httpd:lbstatus>\n",
> > worker->s->lbstatus);
> > ap_rprintf(r,
> > - "
> <httpd:loadfactor>%.2f</httpd:loadfactor>\n",
> > - (float)(worker->s->lbfactor)/100.0);
> > + "
> <httpd:loadfactor>%d</httpd:loadfactor>\n",
> > + worker->s->lbfactor);
> > ap_rprintf(r,
> > " <httpd:transferred>%"
> APR_OFF_T_FMT "</httpd:transferred>\n",
> > worker->s->transferred);
> > @@ -1605,7 +1604,7 @@ static int balancer_handler(request_rec
> > NULL);
> > ap_rvputs(r, "</td><td>",
> > ap_escape_html(r->pool, worker->s-
> >redirect), NULL);
> > - ap_rprintf(r, "</td><td>%.2f</td>", (float)(worker-
> >s->lbfactor)/100.0);
> > + ap_rprintf(r, "</td><td>%d</td>", worker->s-
> >lbfactor);
> > ap_rprintf(r, "<td>%d</td><td>", worker->s->lbset);
> > ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, worker),
> NULL);
> > ap_rputs("</td>", r);
> > @@ -1640,7 +1639,7 @@ static int balancer_handler(request_rec
> > ap_rputs("<form method='POST' enctype='application/x-www-
> form-urlencoded' action='", r);
> > ap_rvputs(r, ap_escape_uri(r->pool, action), "'>\n",
> NULL);
> > ap_rputs("<table><tr><td>Load factor:</td><td><input
> name='w_lf' id='w_lf' type=text ", r);
> > - ap_rprintf(r, "value='%.2f'></td></tr>\n", (float)(wsel-
> >s->lbfactor)/100.0);
> > + ap_rprintf(r, "value='%d'></td></tr>\n", wsel->s-
> >lbfactor);
> > ap_rputs("<tr><td>LB Set:</td><td><input name='w_ls'
> id='w_ls' type=text ", r);
> > ap_rprintf(r, "value='%d'></td></tr>\n", wsel->s->lbset);
> > ap_rputs("<tr><td>Route:</td><td><input name='w_wr'
> id='w_wr' type=text ", r);
> >
> >
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
The only reasons we keep those factors as ints rather than floats are:
1. We can't change the struct fields in 2.4.x
2. The rationale that int based operations will "always"
be faster than floating point
2.5.0 doesn't suffer from #1. We can break API/ABI if there's
a good reason. And #2 is open for debate :)
But yes, the extra granularity (dynamic range) of ldfactor
now means it may take longer to reach steady state for
some methods. We could use the 'age' method to maybe adjust
for that...
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Oct 24, 2017 at 11:58 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> As you said, the external representation as a float is syntactic sugar.
> But the patch breaks that. Someone enters in, for example, 2.50
> and what's displayed in 250, instead of 2.50.
>
> Since all this is normalized by how ldfactor is USED, the algos
> still work, but are now more granular, which is what we wanted.
OK, fair enough (my change was a bad idea).
I'll revert, thanks for the review.
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
As you said, the external representation as a float is syntactic sugar.
But the patch breaks that. Someone enters in, for example, 2.50
and what's displayed in 250, instead of 2.50.
Since all this is normalized by how ldfactor is USED, the algos
still work, but are now more granular, which is what we wanted.
> On Oct 24, 2017, at 4:16 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Tue, Oct 24, 2017 at 3:28 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> I don't understand this patch. It looks like we are no lingering externally
>> representing ldfactor as a float (e.g.: 2.50). Is that right? If so,
>> I'm not sure why...
>
> It seems to me that ldfactor expressed as "float" is just syntactic
> sugar, it's still an "int" in struct proxy_worker_shared.
>
> The lbmethods' algorithms were previously given value between 1 and
> 100, and since 2.4.28 this value is effectively between 100 and 10000.
> For, e.g. bytraffic, this changes the below computation a bit:
>
> mytraffic = ((*worker)->s->transferred / (*worker)->s->lbfactor) +
> ((*worker)->s->read / (*worker)->s->lbfactor);
>
> Meaning that we now have a granularity of 10KB (instead of previous
> 100B, max) before a balancer member can take over the previous one (I
> wondered why several successive small requests were always reaching
> the same backend...).
>
> It's fine that users can now use decimal numbers between 1.0 and 10.0
> (or still non-decimals between 1 and 100 as before, though they
> probably shouldn't mix both), but it shouldn't change lbmethods
> behaviour IMHO.
>
> The other builtin lbmethods don't seem impacted though (they only sum
> up lbfactor), so maybe we need something for bytraffic only...
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Oct 24, 2017, at 4:16 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
>
> Meaning that we now have a granularity of 10KB (instead of previous
> 100B, max) before a balancer member can take over the previous one (I
> wondered why several successive small requests were always reaching
> the same backend...).
>
long-term patterns is what the methods provide... you can't guarantee
short-term behavior.
Re: svn commit: r1813167 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy.c modules/proxy/mod_proxy_balancer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Oct 24, 2017 at 3:28 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> I don't understand this patch. It looks like we are no lingering externally
> representing ldfactor as a float (e.g.: 2.50). Is that right? If so,
> I'm not sure why...
It seems to me that ldfactor expressed as "float" is just syntactic
sugar, it's still an "int" in struct proxy_worker_shared.
The lbmethods' algorithms were previously given value between 1 and
100, and since 2.4.28 this value is effectively between 100 and 10000.
For, e.g. bytraffic, this changes the below computation a bit:
mytraffic = ((*worker)->s->transferred / (*worker)->s->lbfactor) +
((*worker)->s->read / (*worker)->s->lbfactor);
Meaning that we now have a granularity of 10KB (instead of previous
100B, max) before a balancer member can take over the previous one (I
wondered why several successive small requests were always reaching
the same backend...).
It's fine that users can now use decimal numbers between 1.0 and 10.0
(or still non-decimals between 1 and 100 as before, though they
probably shouldn't mix both), but it shouldn't change lbmethods
behaviour IMHO.
The other builtin lbmethods don't seem impacted though (they only sum
up lbfactor), so maybe we need something for bytraffic only...