-
Notifications
You must be signed in to change notification settings - Fork 171
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
Add XDS retry and circuit-breaking functionality to the flow control plugin #1698
base: develop
Are you sure you want to change the base?
Add XDS retry and circuit-breaking functionality to the flow control plugin #1698
Conversation
# whether to use secure protocol to invoke spring cloud downstream service with xds route, example: http or https | ||
enabled-springcloud-xds-route-secure: false | ||
retry: | ||
retryHostPredicate: PreviousHostsPredicate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment
protected abstract Object buildResult(R requestParam, ExecuteContext context); | ||
|
||
/** | ||
* build result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update comment
if (maxRequest != 0 && activeRequestNum > maxRequest) { | ||
context.skip(buildResult(requestParam, context)); | ||
} | ||
businessEntity.setCircuitBusinessName(businessName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setCircuitBreakerBusinessName
String businessName = buildBusinessName(requestParam, businessEntity); | ||
int activeRequestNum = CircuitBreakerManager.incrementActiveRequest(businessName); | ||
int maxRequest = circuitBreakersOptional.get().getMaxRequests(); | ||
if (maxRequest != 0 && activeRequestNum > maxRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When maxrequest is 0, is maxrequest not configured?
@Override | ||
protected String buildBusinessName(Request request, BusinessEntity businessEntity) { | ||
try { | ||
URL url = new URL(request.url()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the entire URL can be directly used as a part of the business, it is not recommended to parse the URL.
} | ||
XdsInstanceCircuitBreakers circuitBreakers = instanceCircuitBreakersOptional.get(); | ||
Response response = (Response) context.getResult(); | ||
if (response == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is difficult to understand. Can you add comments to the following logic?
|| StringUtils.isEmpty(businessEntity.getCircuitBusinessName())) { | ||
return context; | ||
} | ||
handlerFailureRequest(context, businessEntity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the request also a failed request triggered by concurrent circuit breaker? This method is also executed when the circuit breaker is not triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is no trigger, operations such as counting the number of failed requests will be performed.
Map<String, Collection<String>> headers; | ||
Map<String, List<String>> responseHeader = flowControlResult.getResponse().getHeaders(); | ||
if (responseHeader != null) { | ||
headers = new HashMap<>(responseHeader); | ||
} else { | ||
headers = Collections.emptyMap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract as method
if (retryCondition == null) { | ||
continue; | ||
} | ||
boolean needRetry = retryCondition.needRetry(retry, null, conditions, statusCode, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry condition contains the retry status code. Therefore, the retry status code is used to determine whether retry is required.
@@ -32,7 +32,7 @@ | |||
public class ChainContext { | |||
private static final ThreadLocal<Map<String, RequestContext>> THREAD_LOCAL_CONTEXT_MAP = new ThreadLocal<>(); | |||
|
|||
private static final int MAX_SIZE = 8; | |||
private static final int MAX_SIZE = 11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is MAX_SIZE determined? The following describes the reason why MAX_SIZE is 11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the handler is added to the XDS, this does not need to be explained.
value.set(0, value.get(0) + "," + header.getValue()); | ||
continue; | ||
} | ||
headers.put(header.getKey(), Collections.singletonList(header.getValue())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if xdsHeaderOption.isEnabledAppend() is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execute headers.put(header.getKey(), Collections.singletonList(header.getValue()));
} | ||
if (xdsHeaderOption.isEnabledAppend() && headers.containsKey(header.getKey())) { | ||
List<String> value = headers.get(header.getKey()); | ||
value.set(0, value.get(0) + "," + header.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not add the value in list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The append operation is used to concatenate character strings.
What type of PR is this?
Feature
What this PR does / why we need it?
Add XDS retry and circuit-breaking functionality to the flow control plugin
Which issue(s) this PR fixes?
Fixes #1515
Does this PR introduce a user-facing change?
No
Checklist