Skip to content

load balancer: update related methods for load balancer extensions#23474

Merged
wrowe merged 4 commits intoenvoyproxy:mainfrom
wbpcode:dev-load-balancer-policy-base-api
Oct 26, 2022
Merged

load balancer: update related methods for load balancer extensions#23474
wrowe merged 4 commits intoenvoyproxy:mainfrom
wbpcode:dev-load-balancer-policy-base-api

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Oct 13, 2022

Commit Message: load balancer: update related methods for load balancer extensions
Additional Description:

Part of work to land #20634. This is split out from #23472 and is some necessary pre-work for adding load balancer extension.

I made this as an independent PR because this updates some core code and I wish this could be reviewed separately.

Risk Level: mid.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Comment on lines +175 to +179

/**
* @return LoadBalancerPtr a new worker local load balancer.
*/
virtual LoadBalancerPtr create(LoadBalancerParams params) PURE;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is necessary?
If the load_balancing_policy is enabled, all Lb type will be treated as ThreadAwareLoadBalancer. But for Lb type like least request, thread local priority_set and local_priority_set are necessary. So, we must add this params to help the factory to create these types of Lb.

Comment thread envoy/upstream/upstream.h
Comment on lines 842 to 843
*/
virtual const envoy::config::cluster::v3::LoadBalancingPolicy_Policy&
loadBalancingPolicy() const PURE;
virtual const ProtobufTypes::MessagePtr& loadBalancingPolicy() const PURE;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need this change?
Load balancer will be created at runtime in every worker. We shouldn't use non-validated raw Policy to create load balancers. Uncaught exceptions may be throwed.
So, we should unpack and validate this typed config in the configure phase.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 14, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23474 (comment) was created by @wbpcode.

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 14, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23474 (comment) was created by @wbpcode.

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 14, 2022

Hmmm, the ci take too much time run the coverage tests. Seems that the miss-rate of remote cache is very high because I update the upstream.h and local_balancer.h.

Anyway, this PR shouldn't be merged before 1.24.0 release.

/wait

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 22, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23474 (comment) was created by @wbpcode.

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 26, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23474 (comment) was created by @wbpcode.

see: more, trace.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@wrowe wrowe merged commit a0f0ed8 into envoyproxy:main Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants