Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
/test release-notes_api |
| // considerations on choosing an algorithm. | ||
| oneof hash_algorithm { | ||
| // The ring/modulo hash load balancer implements consistent hashing to backend hosts. | ||
| RingHashConfig ring_hash = 6; |
There was a problem hiding this comment.
should we deprecate the minimum_ring_size on L626?
There was a problem hiding this comment.
Yes. We can deprecate. Is it just doc update or any thing else?
| // Please refer to https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#ring-hash | ||
| // and https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#maglev for | ||
| // considerations on choosing an algorithm. | ||
| oneof hash_algorithm { |
There was a problem hiding this comment.
Default is RingHash. I will update.
| // The ring/modulo hash load balancer implements consistent hashing to backend hosts. | ||
| RingHashConfig ring_hash = 6; | ||
| // The Maglev load balancer implements consistent hashing to backend hosts. | ||
| MagLevConfig maglev = 7; |
There was a problem hiding this comment.
I can set maglev without configuring the ring size? If so, how?
maglev: {}?
There was a problem hiding this comment.
yes. you can set maglev:{} and it uses the default table size.
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
/test gencheck_api |
howardjohn
left a comment
There was a problem hiding this comment.
LGTM, just two nits
cc @nmittler
| // load distributions. If the number of hosts in the load balancing | ||
| // pool is larger than the ring size, each host will be assigned a | ||
| // single virtual node. | ||
| uint64 minimum_ring_size = 8 [deprecated=true]; |
There was a problem hiding this comment.
why is this one deprecated? isn't this the new one?
There was a problem hiding this comment.
yeah.. copy pasted at wrong place
| // The table size for Maglev hashing. This helps in controlling the | ||
| // disruption when the backend hosts change. | ||
| // Increasing the table size reduces the amount of disruption. | ||
| uint64 table_size = 9; |
There was a problem hiding this comment.
| uint64 table_size = 9; | |
| uint64 table_size = 1; |
new message, new number scheme (I think? or does nesting change it?). Same above
There was a problem hiding this comment.
No you are right. This has to be follow new number
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
| // pool is larger than the ring size, each host will be assigned a | ||
| // single virtual node. | ||
| uint64 minimum_ring_size = 4; | ||
| uint64 minimum_ring_size = 4 [deprecated=true]; |
There was a problem hiding this comment.
Replace the comment with text indicating that it's deprecated and where to find the replacement value.
| // pool is larger than the ring size, each host will be assigned a | ||
| // single virtual node. | ||
| uint64 minimum_ring_size = 4; | ||
| uint64 minimum_ring_size = 4 [deprecated=true]; |
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
| // Please refer to https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#ring-hash | ||
| // and https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#maglev for | ||
| // considerations on choosing an algorithm. |
There was a problem hiding this comment.
We should give a brief compare here, rather than letting users searching and decide
There was a problem hiding this comment.
I think it is difficult to give concise description in a protobuf comment. That is why Envoy wrote that separate section.
|
@howardjohn @nmittler addressed comments. PTAL |
|
should this have some type of release notes? Else LGTM |
I will add release notes when it is implemented in Istio. |
|
@linsun @howardjohn can you please review this when you get chance? |
As discussed istio/istio#40142 adding support for MagLev loadbalancer.