load balancer: Add a option to configure the table size of maglev#12870
load balancer: Add a option to configure the table size of maglev#12870snowp merged 12 commits intoenvoyproxy:masterfrom
Conversation
Currently, the maglev hash algorithm default to table size to 65537. It is the recommanded size by a paper but it is better if the user can set this value. This patch introduces a new MaglevLbConfig that contains table size of maglev. So, now, the user can set the table size of maglev by their situation. Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
|
|
||
| // Throws an exception if table size is not a prime number. | ||
| TEST_F(MaglevLoadBalancerTest, NoPrimeNumber) { | ||
| EXPECT_THROW_WITH_MESSAGE(init(8), EnvoyException, |
There was a problem hiding this comment.
Even if the new config is added, the table size of Maglev algorithm is not changeable. So, no need to add new tests for that because existing tests are already testing the Maglev algorithm with many table size values.
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
snowp
left a comment
There was a problem hiding this comment.
Thanks this looks pretty good to me, just a couple comments
| // Specific configuration for the :ref:`Maglev<arch_overview_load_balancing_types_maglev>` | ||
| // load balancing policy. | ||
| message MaglevLbConfig { | ||
| // The table size for Maglev hashing. The larger the table size reduces (that is, the more hashes there are for each |
There was a problem hiding this comment.
What is the implication of reducing the disruption of hashing? Would be nice to add some details here.
There was a problem hiding this comment.
Ok. I will add more details.
source/common/upstream/maglev_lb.cc
Outdated
| ENVOY_LOG(debug, "maglev table size: {}", table_size_); | ||
| // The table size should be prime number. | ||
| if (!Primes::isPrime(table_size_)) { | ||
| throw EnvoyException("The table size of maglev should be prime number"); |
There was a problem hiding this comment.
Thanks. :) I will change it.
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
snowp
left a comment
There was a problem hiding this comment.
Thanks this looks pretty good, just some language suggestions
| message MaglevLbConfig { | ||
| // The table size for Maglev hashing. In fact, Maglev aims for ‘minimal disruption’ rather than an absolute guarantee. | ||
| // The minimal disruption means when the set of upstreams changes, a connection will likely be sent to the same | ||
| // upstream as it was before. So, The larger table size reduces disruption of hashing. |
There was a problem hiding this comment.
maybe Increasing the table size reduces the amount of disruption. instead of the So, the phrasing?
| // Specific configuration for the :ref:`Maglev<arch_overview_load_balancing_types_maglev>` | ||
| // load balancing policy. | ||
| message MaglevLbConfig { | ||
| // The table size for Maglev hashing. In fact, Maglev aims for ‘minimal disruption’ rather than an absolute guarantee. |
There was a problem hiding this comment.
I would get rid of the In fact, bit.
| // load balancing policy. | ||
| message MaglevLbConfig { | ||
| // The table size for Maglev hashing. In fact, Maglev aims for ‘minimal disruption’ rather than an absolute guarantee. | ||
| // The minimal disruption means when the set of upstreams changes, a connection will likely be sent to the same |
There was a problem hiding this comment.
Minimal disruption means that when the set ...
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
|
Thank you for your language suggestions. |
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
|
Sorry this seems to have a merge conflict, could you resolve? |
Signed-off-by: DongRyeol Cha <dr83.cha@samsung.com>
Now it is resolved. :) |
|
/lgtm api |
Commit Message:
Currently, the maglev hash algorithm default to table size to 65537.
It is the recommended size by a paper but it is better if the user
can set this value.
This patch introduces a new MaglevLbConfig that contains table
size of maglev.
So, now, the user can set the table size of maglev by their situation.
Signed-off-by: DongRyeol Cha dr83.cha@samsung.com
Additional Description: None
Risk Level: Low
Testing: bazel test
Docs Changes: Add a new option that name is MaglevLbConfig.
Release Notes: Changed current.rst in this PR.