Hi Bhaskar,
sorry for the long delay. Yesterday I found some time to come back to
your patches.
First, I want to thank you for all the well documented work you've done
on this subject, that's really great.
I'm having some comments, both on the implementation and the code itself.
1/4 :
- OK in principle. I have rearranged the doc changes in order not to
overrun the 80-column limit. I also found that naming the new flags
BE_LB_HALG_* was clearer than BE_LB_HF_* since they're only used to
define the algorithm. I have a patch for this that I can merge with
yours if you agree.
- I'm having an minor comment on this code below in cfgparse.c :
+ /* clear the algorithm without discard hash function type */
+ curproxy->lbprm.algo &= (~BE_LB_ALGO | (curproxy->lbprm.algo &
BE_LB_HF_TYPE));
Indeed, it can simply be written this way :
+ curproxy->lbprm.algo &= ~BE_LB_ALGO | BE_LB_HF_TYPE;
Also, I find it a bit confusing that the whole block starting with this
line has been moved to its own block. I think it would have been clearer
if the tests were still organized by hash subject and simply perform the
"curproxy->lbprm.algo &= ~BE_LB_ALGO | BE_LB_HF_TYPE" in each of them just
like there's already "&= ~BE_LB_ALGO" everywhere.
- Just a detail, but the gen_hash() function should probably take the proxy
as the first parameter so that it's immediately clear that it applies to
something related to the proxy. BTW, it will have the same prototype as
the other functions such as get_server_uh() etc this way.
- given the numbers you've shown, I'm still not convinced about the real
benefit of including wt6 here. The initial purpose of such a hash algorithm
was to make it less predictable than others which do a simple multiply or
xor. But if in the end nobody uses it because it's less smooth, there is no
point merging it, what do you think ?
2/4 :
- I really like your principle of keeping the default settings compatible
with current implementations. That said, I still think that since 1.4
does not have the full avalanche with consistent hashing, we should
probably not enable it by default anymore. In fact, 1.4 is the only
stable version right now (and it's the same as the one you're using).
People who use 1.5-dev are used to be careful in their upgrades. So I
think that it would be better to remove the avalanche by default even
when doing a consistent hash. That way it will not change anything for
people migrating from 1.4 to 1.5. And the current 1.5-dev users will be
able to get the same behaviour as today by explicitly adding "avalanche"
on the hash-type line. It will also make the directive behave in a much
more consistent way.
- I would use "BE_LB_HMOD_AVAL" for the name of the hash modifier flag.
3/4 :
- same comment as 1/4 about the computation, otherwise it's obviously OK.
It should be merged into 2/4 since 2/4 alone is not correct without it.
4/4 :
- OK as well, modulto the 80-char limit which I have already fixed in the
review I started yesterday.
Now what I can propose :
- I adjust the doc in your first patch to fix the 80-column limit ;
- I rename the flags as mentionned above ;
- I merge 2/4 with 3/4
- I remove WT6 for now
- I disable avalanche for consistent only (and adjust the doc accordingly)
- And I merge everything on my own. It's the easiest thing to do for me at
the moment.
Alternative : you want to rework some things and to submit another round.
Note, I found that your spreadsheet was very useful and it would be nice to
have either a link to it in the commit (if the link is permanent, I don't
know) or just a text-only copy of it in the "tests" directory.
Thanks!
Willy
