+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
