Hello Willy, Thanks for your comments,
> 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. Sounds good. > - 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; Yep correct, I do not know why I did that :-) > 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. OK, I did move the block specifically to avoid repeating the line, but am happy to have it either way. > - 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. +1. I was not paying attention to the order of function arguments. proxy as first definitely makes sense. > - 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 ? No concerns at all. I/We do only really care about DJB2. I left it in there so that you had an opportunity to take a look at the code + results and decide. > 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 have no concerns over this. I might have misunderstood one of our conversations as implying that backward compatibility was a requirement. If anything the code in cfgparse is a little more confusing to understand when attempting to maintain backwards compatibility. > 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. +1. Yes, I spent about 1/2 a day pulling my hair on this before I realized what I was doing wrong. > 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. Unless you would like me to make the changes, I am more than happy for you to merge in everything, as it seems you have already made the edits. > 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. I will send you a CSV to add to tests later today, that is the safe bet on this. Once again, thank you. Thanks Bhaskar On Wed, Nov 13, 2013 at 8:34 AM, Willy Tarreau <[email protected]> wrote: > 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 > >
