subject:"svn commit\: r1828890 \- in \/httpd\/httpd\/trunk\: .\/ docs\/log\-message\-tags\/ docs\/manual\/howto\/ docs\/manual\/mod\/ modules\/proxy\/ modules\/proxy\/balancers\/"

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-13 Thread Ruediger Pluem



On 04/11/2018 02:11 PM, jhri...@apache.org wrote:
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
> 
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.
> 
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/howto/reverse_proxy.xml
> httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml
> httpd/httpd/trunk/modules/proxy/balancers/mod_lbmethod_bybusyness.c
> httpd/httpd/trunk/modules/proxy/balancers/mod_lbmethod_byrequests.c
> httpd/httpd/trunk/modules/proxy/balancers/mod_lbmethod_bytraffic.c
> httpd/httpd/trunk/modules/proxy/mod_proxy.c
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> 

> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1828890=1828889=1828890=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Apr 11 12:11:05 2018

> @@ -1293,6 +1294,121 @@ PROXY_DECLARE(apr_status_t) ap_proxy_ini
>  return APR_SUCCESS;
>  }
>  
> +PROXY_DECLARE(proxy_worker *) 
> ap_proxy_balancer_get_best_worker(proxy_balancer *balancer,
> +request_rec 
> *r,
> +
> proxy_is_best_callback_fn_t *is_best,
> +void *baton)
> +{
> +int i = 0;
> +int cur_lbset = 0;
> +int max_lbset = 0;
> +int unusable_workers = 0;
> +apr_pool_t *tpool = NULL;
> +apr_array_header_t *spares = NULL;
> +apr_array_header_t *standbys = NULL;
> +proxy_worker *worker = NULL;
> +proxy_worker *best_worker = NULL;
> +
> +ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server, APLOGNO(10122)
> + "proxy: Entering %s for BALANCER (%s)",
> + balancer->lbmethod->name, balancer->s->name);
> +
> +apr_pool_create(, r->pool);
> +
> +spares = apr_array_make(tpool, 1, sizeof(proxy_worker*));
> +standbys = apr_array_make(tpool, 1, sizeof(proxy_worker*));
> +
> +/* Process lbsets in order, only replacing unusable workers in a given 
> lbset
> + * with available spares from the same lbset. Hot standbys will be used 
> as a
> + * last resort when all other workers and spares are unavailable.
> + */
> +for (cur_lbset = 0; !best_worker && (cur_lbset <= max_lbset); 
> cur_lbset++) {
> +unusable_workers = 0;
> +apr_array_clear(spares);
> +apr_array_clear(standbys);
> +
> +for (i = 0; i < balancer->workers->nelts; i++) {
> +worker = APR_ARRAY_IDX(balancer->workers, i, proxy_worker *);
> +
> +if (worker->s->lbset > max_lbset) {
> +max_lbset = worker->s->lbset;
> +}
> +
> +if (worker->s->lbset != cur_lbset) {
> +continue;
> +}
> +
> +/* A draining worker that is neither a spare nor a standby 
> should be
> + * considered unusable to be replaced by spares.
> + */
> +if (PROXY_WORKER_IS_DRAINING(worker)) {
> +if (!PROXY_WORKER_IS_SPARE(worker) && 
> !PROXY_WORKER_IS_STANDBY(worker)) {
> +unusable_workers++;
> +}
> +
> +continue;
> +}
> +
> +/* If the worker is in error state run retry on that worker. It 
> will
> + * be marked as operational if the retry timeout is elapsed.  The
> + * worker might still be unusable, but we try anyway.
> + */
> +if (!PROXY_WORKER_IS_USABLE(worker)) {
> +ap_proxy_retry_worker("BALANCER", worker, r->server);
> +}
> +
> +if (PROXY_WORKER_IS_SPARE(worker)) {
> +if (PROXY_WORKER_IS_USABLE(worker)) {
> +APR_ARRAY_PUSH(spares, proxy_worker *) = worker;
> +}
> +}
> +else if (PROXY_WORKER_IS_STANDBY(worker)) {
> +if (PROXY_WORKER_IS_USABLE(worker)) {
> +APR_ARRAY_PUSH(standbys, proxy_worker *) = worker;
> +}
> +}
> +else if (PROXY_WORKER_IS_USABLE(worker)) {
> +  if (is_best(worker, best_worker, baton)) {
> +best_worker = worker;
> +  }
> +}
> +

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-13 Thread Ruediger Pluem

On 04/11/2018 04:23 PM, Jim Jagielski wrote:
> +1 on keeping the patch as is and not breaking it out as a "refactor" plus
> a new feature.

Definitely for this case. OTOH if practically possible I like to see stuff in 
separate commits to ease reviewing and
later error diagnosis. But as said this is not always practical and is always a 
trade-off.

> 
> We have seen WAY too many cases where a refactor has caused
> issues. I'm not saying we shouldn't fix things, but major refactors
> should, IMO, have a real-world need, and tangible improvement,
> other than "I want it to work this way."

Agreed, but real-world needs IMHO include performance improvements and better 
readability / maintainability of code and
as such are not not necessarily functional changes. But I am well aware that 
what is more readable or maintainable is
easily a subject of personal taste :-)

Regards

Rüdiger

Re: Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

2018-04-11 Thread Jim Jagielski

It's been awhile but I think hot standbys were added before we came
up with the idea of lb sets... And since people were using hot
standbys, we didn't want to force them to change their configs.

We can drop hot-standbys in trunk/2.5/2.6, but we'll need to keep
them in 2.4.

> On Apr 11, 2018, at 10:57 AM, Jim Riggs  wrote:
> 
> On 11 Apr 2018, at 07:11, jhri...@apache.org wrote:
>> 
>> Author: jhriggs
>> Date: Wed Apr 11 12:11:05 2018
>> New Revision: 1828890
>> 
>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>> Log:
>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>> Hot spare members are
>> used as drop-in replacements for unusable workers in the same load balancer 
>> set. This differs
>> from hot standbys which are only used when all workers in a set are 
>> unusable. PR 61140.
> 
> Speaking of balancer member types, hot spares (new), and hot standbys 
> (existing), is there really a need for hot standbys? When I first started 
> using mod_proxy_balancer many years ago, I actually thought/assumed that "hot 
> standbys" had the behavior that the new spares now have (i.e. drop-in 
> replacements for unavailable workers) until I studied the documentation more 
> carefully.
> 
> Standbys are really superfluous, since the same behavior can be achieved by 
> sets. For example, the behavior of the following will be exactly the same:
> 
> 
>  ProxySet lbmethod=bytraffic
>  BalancerMember "http://server1/; lbset=0
>  BalancerMember "http://server2/; lbset=0
>  BalancerMember "http://server3/; lbset=0 status=+H
>  BalancerMember "http://server4/; lbset=1
>  BalancerMember "http://server5/; lbset=1
>  BalancerMember "http://server6/; lbset=1 status=+H
> 
> 
> 
>  ProxySet lbmethod=bytraffic
>  BalancerMember "http://server1/; lbset=0
>  BalancerMember "http://server2/; lbset=0
>  BalancerMember "http://server3/; lbset=1
>  BalancerMember "http://server4/; lbset=2
>  BalancerMember "http://server5/; lbset=2
>  BalancerMember "http://server6/; lbset=3
> 
> 
> So, is there really any reason to keep hot standbys? Do we consider the 
> concept of the +H flag to be easier to understand or use than LB sets? If so, 
> do we just need better docs? Am I missing any nuances of the H flag?
> 
> I would love to see standbys go away, personally.
> 
> - Jim
>

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-11 Thread Yann Ylavic

I did not advocate refactor for refactoring sake, was just a remark
about (possibly) staging "big" changes.
Here for instance ap_proxy_balancer_get_best_worker() refactor could
have been done on the existing code first, and then the code needed
for hot spare members be added to that common function next.

That could have helped both review and bissection: if something stops
working, does it come from the refactor or the new feature?
But I'm not asking to revert and redo staged commits though, will try
to figure out ;)


On Wed, Apr 11, 2018 at 4:23 PM, Jim Jagielski  wrote:
> +1 on keeping the patch as is and not breaking it out as a "refactor" plus
> a new feature.
>
> We have seen WAY too many cases where a refactor has caused
> issues. I'm not saying we shouldn't fix things, but major refactors
> should, IMO, have a real-world need, and tangible improvement,
> other than "I want it to work this way."
>
> Too often people just refactor for refactoring sake. That's not a
> Good Idea, IMO.
>
>
> On Apr 11, 2018, at 10:09 AM, Jim Riggs  wrote:
>
> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
>
> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
>
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R).
> Hot spare members are
> used as drop-in replacements for unusable workers in the same load balancer
> set. This differs
> from hot standbys which are only used when all workers in a set are
> unusable. PR 61140.
>
>
> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
>
> Maybe it could have been a separate commit than the spare members
> addition though, not mixing refactoring and features.
> Staging helps review IHMO.
>
>
> Thanks for the feedback, Yann!
>
> I never really viewed ap_proxy_balancer_get_best_worker() as a refactor
> separate from the hot spare change that required it, though I suppose it
> definitely could have stood alone. It did remove a lot of duplicate code.
> Basically, that commit is just an existing patch that has been out there
> floating around since ACNA last year that I just updated to the current
> codebase. In the dev@ thread at the time, we never discussed splitting them
> either.
>
> Regardless, I'm glad to have the hot spare functionality that several of us
> have always thought was missing, and I'm glad I didn't appear to b0rk the
> entire repo in the process.
>
> Thanks for taking it easy on the new guy. ;-)
>
> - Jim
>
>

Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

2018-04-11 Thread Jim Riggs

On 11 Apr 2018, at 07:11, jhri...@apache.org wrote:
> 
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
> 
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.

Speaking of balancer member types, hot spares (new), and hot standbys 
(existing), is there really a need for hot standbys? When I first started using 
mod_proxy_balancer many years ago, I actually thought/assumed that "hot 
standbys" had the behavior that the new spares now have (i.e. drop-in 
replacements for unavailable workers) until I studied the documentation more 
carefully.

Standbys are really superfluous, since the same behavior can be achieved by 
sets. For example, the behavior of the following will be exactly the same:

  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=0 status=+H
  BalancerMember "http://server4/; lbset=1
  BalancerMember "http://server5/; lbset=1
  BalancerMember "http://server6/; lbset=1 status=+H

  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=1
  BalancerMember "http://server4/; lbset=2
  BalancerMember "http://server5/; lbset=2
  BalancerMember "http://server6/; lbset=3

So, is there really any reason to keep hot standbys? Do we consider the concept 
of the +H flag to be easier to understand or use than LB sets? If so, do we 
just need better docs? Am I missing any nuances of the H flag?

I would love to see standbys go away, personally.

- Jim

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-11 Thread Jim Jagielski

+1 on keeping the patch as is and not breaking it out as a "refactor" plus
a new feature.

We have seen WAY too many cases where a refactor has caused
issues. I'm not saying we shouldn't fix things, but major refactors
should, IMO, have a real-world need, and tangible improvement,
other than "I want it to work this way."

Too often people just refactor for refactoring sake. That's not a
Good Idea, IMO.

> On Apr 11, 2018, at 10:09 AM, Jim Riggs  wrote:
> 
>> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
>> 
>> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>>> Author: jhriggs
>>> Date: Wed Apr 11 12:11:05 2018
>>> New Revision: 1828890
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>>> Log:
>>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>>> Hot spare members are
>>> used as drop-in replacements for unusable workers in the same load balancer 
>>> set. This differs
>>> from hot standbys which are only used when all workers in a set are 
>>> unusable. PR 61140.
>> 
>> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
>> 
>> Maybe it could have been a separate commit than the spare members
>> addition though, not mixing refactoring and features.
>> Staging helps review IHMO.
> 
> Thanks for the feedback, Yann!
> 
> I never really viewed ap_proxy_balancer_get_best_worker() as a refactor 
> separate from the hot spare change that required it, though I suppose it 
> definitely could have stood alone. It did remove a lot of duplicate code. 
> Basically, that commit is just an existing patch that has been out there 
> floating around since ACNA last year that I just updated to the current 
> codebase. In the dev@ thread at the time, we never discussed splitting them 
> either.
> 
> Regardless, I'm glad to have the hot spare functionality that several of us 
> have always thought was missing, and I'm glad I didn't appear to b0rk the 
> entire repo in the process.
> 
> Thanks for taking it easy on the new guy. ;-)
> 
> - Jim

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-11 Thread Jim Riggs

> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
> 
> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>> Author: jhriggs
>> Date: Wed Apr 11 12:11:05 2018
>> New Revision: 1828890
>> 
>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>> Log:
>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>> Hot spare members are
>> used as drop-in replacements for unusable workers in the same load balancer 
>> set. This differs
>> from hot standbys which are only used when all workers in a set are 
>> unusable. PR 61140.
> 
> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
> 
> Maybe it could have been a separate commit than the spare members
> addition though, not mixing refactoring and features.
> Staging helps review IHMO.

Thanks for the feedback, Yann!

I never really viewed ap_proxy_balancer_get_best_worker() as a refactor 
separate from the hot spare change that required it, though I suppose it 
definitely could have stood alone. It did remove a lot of duplicate code. 
Basically, that commit is just an existing patch that has been out there 
floating around since ACNA last year that I just updated to the current 
codebase. In the dev@ thread at the time, we never discussed splitting them 
either.

Regardless, I'm glad to have the hot spare functionality that several of us 
have always thought was missing, and I'm glad I didn't appear to b0rk the 
entire repo in the process.

Thanks for taking it easy on the new guy. ;-)

- Jim

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

2018-04-11 Thread Yann Ylavic

On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
>
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.

Nice ap_proxy_balancer_get_best_worker() simplification Jim.

Maybe it could have been a separate commit than the spare members
addition though, not mixing refactoring and features.
Staging helps review IHMO.

Thanks anyway,
Yann.

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

Re: Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

8 matches

Site Navigation

Mail list logo

Footer information