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 <j...@jagunet.com> 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 <jim...@riggs.me> wrote:
>
> On 11 Apr 2018, at 08:28, Yann Ylavic <ylavic....@gmail.com> wrote:
>
> On Wed, Apr 11, 2018 at 2: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&view=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
>
>

Reply via email to