Use FNV instead of SHA512 for the hashing in A/B test #7260
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7260 +/- ##
=========================================
- Coverage 58.34% 58.3% -0.05%
=========================================
Files 493 493
Lines 21661 21674 +13
Branches 3486 3487 +1
=========================================
- Hits 12639 12636 -3
- Misses 8242 8257 +15
- Partials 780 781 +1
Continue to review full report at Codecov.
|
|
we should include the LS experiment to use the sha512 algorithm as well cc @gramster |
kimadeline
left a comment
kimadeline
left a comment
There was a problem hiding this comment.
- Can you open a code health issue to remove the SHA512 hashing code when we retire all the old experiments?
- What was the final answer for your message about the distribution test?
I have written a test, verifying
- on small changes in input, the difference between outputs is large on average (~ 33% in our case)
- average hit per bucket is as expected
- hits for each bucket should be close to average hit
But still in worst case, this could be flaky. So I think we should maybe not include this test, as we have verified each fact manually.
Here #7333
He didn't object to it, so I guess he agrees. You may want to ask him again. |
|
Please do add a dispersion unit test. If you (say) hash each of the integers from 1 to 10000 (converted to strings), then you want to test that no bucket has less than 75 hits. That should be sufficient and is very unlikely to fail. |
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
LGTM
I only have a few mild comments.
| } | ||
| } | ||
| // Total number of words = wordList.length * 10, because we added ten variants of each word above. | ||
| const expectedHitsPerBucket = wordList.length * 10 / 100; |
There was a problem hiding this comment.
It looks like some of the items in wordList are ignored. Shouldn't that be reflected here?
There was a problem hiding this comment.
Actually we don't need to ignore anything, so removing that line.
|
@ericsnowcurrently Addressed most of the comments. I found the other minor comments mainly stylistic, so leaving those. |
6 participants
For #7218
[ ] Test plan is updated as appropriatepackage-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)[ ] The wiki is updated with any design decisions/details.