FIX: Editing ChangeLog and Readme for CallingServer. #23167
Conversation
cochi2
requested review from
ankitarorabit,
JianpingChen,
minnieliu and
a team
as code owners
ghost
added
the
| && !locations.contains(response.getHeaderValue(LOCATION_HEADER_NAME)) | ||
| && redirectNumber < MAX_REDIRECTS; | ||
| private boolean isRedirectResponse(HttpResponse response) { | ||
| return response.getStatusCode() == SC_MOVED_TEMPORARILY || response.getStatusCode() == SC_MOVED_PERMANENTLY; |
There was a problem hiding this comment.
307 (temporary redirect) and 308 (permanent redirect) should be redirected as well.
| } | ||
| if (attemptedRedirectLocations.contains(response.getHeaderValue(LOCATION_HEADER_NAME))) { | ||
| logger.error(String.format("Request to %s was redirected more than once to: %s", | ||
| context.getHttpRequest().getUrl(), response.getHeaderValue(LOCATION_HEADER_NAME))); |
There was a problem hiding this comment.
Is it also worth printing the path here to trace back to which two URLs are redirected to the same location? It will help in troubleshooting. For this, you might have to keep track of the order of the locations by using a List instead of a Set.
| return next.clone().process().flatMap(httpResponse -> { | ||
| if (shouldRedirect(httpResponse, redirectNumber, locations)) { | ||
| if (isRedirectResponse(httpResponse) |
There was a problem hiding this comment.
For 302 (moved permanently) and 308 (permanent redirect), we should consider caching the redirect location of a request URL and go there directly on the first attempt for the next request. For temporary redirects too, we might want to cache for a certain time interval.
...ation-common/src/main/java/com/azure/communication/common/implementation/RedirectPolicy.java
Outdated
Show resolved
Hide resolved
|
@alzimmermsft and @samvaity - some of my comments in this PR should be addressed when we move the redirect policy to azure-core. |
Also, Applying new comments received for the RedirectPolicy
cochi2
force-pushed
the
fmorales/redirect_changes
branch
from
0de38ba to
a5f141b
Compare
cochi2
force-pushed
the
fmorales/redirect_changes
branch
from
a5f141b to
ec4bbe4
Compare
|
Hello @cochi2! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
Also, Applying new comments received for the RedirectPolicy