Fix FNV1a key poor distribution #208
Merged
+138
−1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two fixes included in this PR which together improve key distribution in the
DefaultNodeLocator
significantly.Fix 1: The FNV hashes in FnvHash don't initialize themselves when the hash objects are constructed, causing the first hashes to be generated out of them to have an under-randomized value. It seems that these were developed with the assumption that either the
Initialize
method would be called by the builtinComputeHash
method (which it is after the first hash is computed, to prep for the next hash), or that theIUIntHashAlgorithm.ComputeHash
method would be used which performs the init. But neither is true in theDefaultNodeLocator
.This change is supported by an additional test that verifies the FNV1a algorithm is matching published test vectors.
Fix 2: I've been noticing fairly uneven distribution of keys to multiple nodes in 3, 6, 9, and 12 node memcached clusters. I've tracked that down to the implementation of
DefaultNodeLocator
, which populates a consistent hash ring by using an FNV1a hash algorithm and taking the memcached node hostname and adding suffixed values like "-1", "-2", ... "-100" to create 100 hash positions in that ring.The test
DefaultNodeLocatorTest.TestLocator
measures the variability from ideal with the implemented DefaultNodeLocator; eg. if you had 10 keys and 10 servers, ideally you'd get 1 key per server, variability 0%. The current implementation has a variability of 76% in this test; with 1,000,000 keys distributed over 8 servers, the luckiest server gets only 52,897 keys, and the unluckiest server gets 219,437 keys, where the ideal server would get 125,000 keys.The root cause of this problem is that the FNV-1a hash algorithm is insensitive to changes at the end of the string:
And modifying the end of the string is exactly what the node locator does for populating the consistent hash ring.
To address this, I've made a simple change in principal: I've changed the node ring to be populated as "1-hostname", "2-hostname", etc.
Here's a comparison of different options I evaluated:
This change seemed like the least risk, lowest code change with the best outcome.
There is one risk to communicate with this change -- when deployed in an existing application, the key->node distributions will be completely altered, resulting in a high initial rate of cache misses. The distribution fix is probably worth it, but that could be a risk that library users would want to manage.