upstream: include locality_weights when the cluster uses a load balancing extension#17947
Conversation
balancing extension `locality_weights` is currently calculated only when the cluster contains a `common_lb_config` containing a locality weighted config. This allows load balancing extensions to receive `locality_weights`. In the future, it might be preferable to have the load balancing policy indicate, programatically, whether it needs the locality weights, as an optimization in cases where it doesn't. This commit also pulls the custom LB used in `CustomStaticCluster` into its own library so that it can be re-used. Signed-off-by: Eugene Chan <eugenechan@google.com>
Signed-off-by: Eugene Chan <eugenechan@google.com>
|
@snowp would you be able to help review this one? It's a pretty small fix and will help us out in our use of LB extensions. Thanks. |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
This seems reasonable to me, just a small comment
| @@ -1472,7 +1472,9 @@ void PriorityStateManager::updateClusterPrioritySet( | |||
| std::vector<HostVector> per_locality; | |||
|
|
|||
| // If we are configured for locality weighted LB we populate the locality weights. | |||
There was a problem hiding this comment.
Maybe update this comment with the reasoning for including this for lb policies? My understand is that the extension may want to use this, so we include this just in case. Maybe even a TODO to have this happen only if the extension requests it somehow
There was a problem hiding this comment.
Done and done. Your understanding matches mine - we don't have a way to indicate, from the LB extension, whether we want the locality weights here, since we can't tell simply by inspecting the config. The ideal way to solve this problem, I think, is programatically - have the LB extension indicate whether it wants the locality weights. But I think that would require the logic here to move to something in the load balancing policy, whereas it's currently somewhere enclosed by cluster logic.
Signed-off-by: Eugene Chan <eugenechan@google.com>
|
Seems like a CI failure, can you take a look? /wait |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Eugene Chan <eugenechan@google.com>
|
Ha, I didn't realize that CI checked my spelling. PTAL :-) |
…cing extension (envoyproxy#17947) `locality_weights` is currently calculated only when the cluster contains a `common_lb_config` containing a locality weighted config. This allows load balancing extensions to receive `locality_weights`. In the future, it might be preferable to have the load balancing policy indicate, programatically, whether it needs the locality weights, as an optimization in cases where it doesn't. This commit also pulls the custom LB used in `CustomStaticCluster` into its own library so that it can be re-used. Signed-off-by: Eugene Chan <eugenechan@google.com> Signed-off-by: pianiststickman <34144687+pianiststickman@users.noreply.github.com>
4 participants
locality_weightsis currently calculated only when the cluster contains acommon_lb_configcontaining a locality weighted config. This allows load balancing extensions to receivelocality_weights.In the future, it might be preferable to have the load balancing policy indicate, programatically, whether it needs the locality weights, as an optimization in cases where it doesn't.
This commit also pulls the custom LB used in
CustomStaticClusterinto its own library so that it can be re-used.Signed-off-by: Eugene Chan eugenechan@google.com
Commit Message: upstream: include locality_weights when the cluster uses a load balancing extension
Additional Description:
locality_weightsis currently calculated only when the cluster contains acommon_lb_configcontaining a locality weighted config. This allows load balancing extensions to receivelocality_weights.In the future, it might be preferable to have the load balancing policy indicate, programatically, whether it needs the locality weights, as an optimization in cases where it doesn't.
This commit also pulls the custom LB used in
CustomStaticClusterinto its own library so that it can be re-used.Risk Level: low
Testing: new unit test
Docs Changes: n/a
Release Notes: includes
locality_weightsfor load balancing extensionsPlatform Specific Features: n/a