Hello Willy, I wanted to ask you permission, on whether it was ok to blog about this change, once it was committed on Tumblr's engineering blog [1]. I can do so after the changes are in and have your permission on it.
Thanks Bhaskar [1] http://engineering.tumblr.com/ On Wed, Nov 13, 2013 at 9:41 AM, Bhaskar Maddala <[email protected]> wrote: > 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 >> >> >
