Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: implement xDS circuit breaking max_requests #7517

Merged
merged 13 commits into from
Nov 2, 2020

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Oct 13, 2020

No description provided.

Comment on lines +428 to +429
if (result.getStatus().isOk() && result.getSubchannel() != null) {
if (requestCount.get() >= maxConcurrentRequests) {
Copy link
Member

Choose a reason for hiding this comment

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

If result is a buffering result, should we drop the request immediately as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result.getSubchannel() != null should be the thing to cover it.

Copy link
Member

Choose a reason for hiding this comment

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

I think for buffering result (result.getStatus().isOk() && result.getSubchannel() == null), it should also be dropped immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It will cause RPCs to be dropped prematurely. We shouldn't do anything to buffer result. The channel will handle it correctly.

Copy link
Member

Choose a reason for hiding this comment

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

If requestCount.get() >= maxConcurrentRequest it should drop the RPC regardless of the subchannel's readiness. What's dropped prematurely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that should not count toward the number of dropped requests. Also, it doesn't make sense to override the non-ready picker with a drop picker.

@voidzcy
Copy link
Contributor Author

voidzcy commented Oct 30, 2020

@dapengzhang0 Ping.

@voidzcy voidzcy merged commit 47d1488 into grpc:master Nov 2, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
Implemented xDS circuit breaking for the maximum number of requests can be in-flight. The threshold is retrieved from CDS responses and is configured at the cluster level. It is implemented by wrapping the Picker spawned by EDS LB policy (which resolves endpoints for a single cluster) with stream-limiting logic. That is, when the picker is trying to create a new stream (aka, a new call), it is controlled by the number of open streams created by the current EDS LB policy. RPCs dropped by circuit breakers are recorded into total number of drops at cluster level and will be reported to TD via LRS.

In the future, multiple gRPC channels can be load balancing requests to the same (global) cluster. Those request should share the same quota for maximum number of requests can be in-flight. We will use a global counter for aggregating the number of currently-in-flight requests per cluster.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants