load balancer: Add a option to configure the table size of maglev #12870

chadr123 · 2020-08-28T10:05:44Z

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 [email protected]
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.

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 <[email protected]>

Signed-off-by: DongRyeol Cha <[email protected]>

repokitteh-read-only · 2020-08-28T10:05:50Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12870 was opened by chadr123.

see: more, trace.

chadr123 · 2020-08-28T10:08:59Z

test/common/upstream/maglev_lb_test.cc

@@ -63,6 +66,12 @@ TEST_F(MaglevLoadBalancerTest, NoHost) {
  EXPECT_EQ(nullptr, lb_->factory()->create()->chooseHost(nullptr));
 };

+// Throws an exception if table size is not a prime number.
+TEST_F(MaglevLoadBalancerTest, NoPrimeNumber) {
+  EXPECT_THROW_WITH_MESSAGE(init(8), EnvoyException,


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.

chadr123 · 2020-08-28T11:46:51Z

/retest

repokitteh-read-only · 2020-08-28T11:46:55Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12870 (comment) was created by @chadr123.

see: more, trace.

Signed-off-by: DongRyeol Cha <[email protected]>

htuch

/lgtm api

snowp

Thanks this looks pretty good to me, just a couple comments

snowp · 2020-08-28T19:32:09Z

api/envoy/config/cluster/v3/cluster.proto

+  // 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


What is the implication of reducing the disruption of hashing? Would be nice to add some details here.

Ok. I will add more details.

snowp · 2020-08-28T19:34:33Z

source/common/upstream/maglev_lb.cc

+  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");


nit: should -> must

Thanks. :) I will change it.

chadr123 · 2020-08-28T23:35:49Z

/retest

repokitteh-read-only · 2020-08-28T23:35:53Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12870 (comment) was created by @chadr123.

see: more, trace.

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 · 2020-08-29T14:56:50Z

/retest

repokitteh-read-only · 2020-08-29T14:56:54Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12870 (comment) was created by @chadr123.

see: more, trace.

Signed-off-by: DongRyeol Cha <[email protected]>

snowp

Thanks this looks pretty good, just some language suggestions

snowp · 2020-08-31T16:38:42Z

api/envoy/config/cluster/v3/cluster.proto

+  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.


maybe Increasing the table size reduces the amount of disruption. instead of the So, the phrasing?

snowp · 2020-08-31T16:39:11Z

api/envoy/config/cluster/v3/cluster.proto

+  // 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.


I would get rid of the In fact, bit.

snowp · 2020-08-31T16:39:30Z

api/envoy/config/cluster/v3/cluster.proto

+  // 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


Minimal disruption means that when the set ...

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 · 2020-09-01T04:10:58Z

Thank you for your language suggestions.
It has been update. Please check it again. :)

chadr123 · 2020-09-01T07:21:10Z

/retest

repokitteh-read-only · 2020-09-01T07:21:15Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12870 (comment) was created by @chadr123.

see: more, trace.

snowp

Thanks this LGTM!

@htuch This needs another API approval

snowp · 2020-09-02T21:47:10Z

Sorry this seems to have a merge conflict, could you resolve?

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 · 2020-09-03T04:51:01Z

Sorry this seems to have a merge conflict, could you resolve?

Now it is resolved. :)

htuch · 2020-09-03T18:54:11Z

/lgtm api

snowp

Thanks!

DongRyeol Cha added 3 commits August 25, 2020 12:40

Merge branch 'master' into table_size_maglev

91a370e

Signed-off-by: DongRyeol Cha <[email protected]>

Fix tests

bc7ea00

Signed-off-by: DongRyeol Cha <[email protected]>

repokitteh-read-only bot added the api label Aug 28, 2020

chadr123 commented Aug 28, 2020

View reviewed changes

DongRyeol Cha added 2 commits August 28, 2020 21:06

Merge branch 'master' into table_size_maglev

ed490d5

Signed-off-by: DongRyeol Cha <[email protected]>

Fix ci

153a0f4

Signed-off-by: DongRyeol Cha <[email protected]>

mattklein123 assigned snowp Aug 28, 2020

htuch reviewed Aug 28, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 28, 2020

snowp suggested changes Aug 28, 2020

View reviewed changes

DongRyeol Cha added 2 commits August 29, 2020 19:59

Merge branch 'master' into table_size_maglev

92e59ef

Signed-off-by: DongRyeol Cha <[email protected]>

Address comments

2fd8194

Signed-off-by: DongRyeol Cha <[email protected]>

repokitteh-read-only bot added the api label Aug 29, 2020

chadr123 requested a review from snowp August 29, 2020 15:41

chadr123 marked this pull request as ready for review August 29, 2020 15:42

Add more tests

5f3a1df

Signed-off-by: DongRyeol Cha <[email protected]>

snowp suggested changes Aug 31, 2020

View reviewed changes

DongRyeol Cha added 2 commits September 1, 2020 12:07

Address some language suggestions

ffb2486

Signed-off-by: DongRyeol Cha <[email protected]>

Merge branch 'master' into table_size_maglev

0da903f

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 requested a review from snowp September 1, 2020 04:11

snowp previously approved these changes Sep 2, 2020

View reviewed changes

Merge branch 'master' into table_size_maglev

f89e237

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 dismissed snowp’s stale review via f89e237 September 3, 2020 01:38

Fix ci

ead324d

Signed-off-by: DongRyeol Cha <[email protected]>

chadr123 requested review from htuch and snowp September 3, 2020 04:51

repokitteh-read-only bot removed the api label Sep 3, 2020

snowp approved these changes Sep 3, 2020

View reviewed changes

snowp merged commit 5fd73ca into envoyproxy:master Sep 3, 2020

chadr123 deleted the table_size_maglev branch September 3, 2020 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #12870

chadr123 commented Aug 28, 2020 •

edited

Loading

repokitteh-read-only bot commented Aug 28, 2020

chadr123 Aug 28, 2020 •

edited

Loading

chadr123 commented Aug 28, 2020

repokitteh-read-only bot commented Aug 28, 2020

htuch left a comment

snowp left a comment

snowp Aug 28, 2020

chadr123 Aug 29, 2020

snowp Aug 28, 2020

chadr123 Aug 29, 2020

chadr123 commented Aug 28, 2020

repokitteh-read-only bot commented Aug 28, 2020

chadr123 commented Aug 29, 2020

repokitteh-read-only bot commented Aug 29, 2020

snowp left a comment

snowp Aug 31, 2020

snowp Aug 31, 2020

snowp Aug 31, 2020

chadr123 commented Sep 1, 2020

chadr123 commented Sep 1, 2020

repokitteh-read-only bot commented Sep 1, 2020

snowp left a comment

snowp commented Sep 2, 2020

chadr123 commented Sep 3, 2020

htuch commented Sep 3, 2020

snowp left a comment

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 #12870

Conversation

chadr123 commented Aug 28, 2020 • edited Loading

repokitteh-read-only bot commented Aug 28, 2020

chadr123 Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

chadr123 commented Aug 28, 2020

repokitteh-read-only bot commented Aug 28, 2020

htuch left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

snowp Aug 28, 2020

Choose a reason for hiding this comment

chadr123 Aug 29, 2020

Choose a reason for hiding this comment

snowp Aug 28, 2020

Choose a reason for hiding this comment

chadr123 Aug 29, 2020

Choose a reason for hiding this comment

chadr123 commented Aug 28, 2020

repokitteh-read-only bot commented Aug 28, 2020

chadr123 commented Aug 29, 2020

repokitteh-read-only bot commented Aug 29, 2020

snowp left a comment

Choose a reason for hiding this comment

snowp Aug 31, 2020

Choose a reason for hiding this comment

snowp Aug 31, 2020

Choose a reason for hiding this comment

snowp Aug 31, 2020

Choose a reason for hiding this comment

chadr123 commented Sep 1, 2020

chadr123 commented Sep 1, 2020

repokitteh-read-only bot commented Sep 1, 2020

snowp left a comment

Choose a reason for hiding this comment

snowp commented Sep 2, 2020

chadr123 commented Sep 3, 2020

htuch commented Sep 3, 2020

snowp left a comment

Choose a reason for hiding this comment

chadr123 commented Aug 28, 2020 •

edited

Loading

chadr123 Aug 28, 2020 •

edited

Loading