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

[Management] Fix print error log when server return redirect (http code 307). #14259

Merged
merged 3 commits into from
Feb 14, 2022
Merged

[Management] Fix print error log when server return redirect (http code 307). #14259

merged 3 commits into from
Feb 14, 2022

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Feb 13, 2022

Motivation

Currently, some RestAPI will print an error log when the server returns the redirect code(307). which can let the user a lit bit confusing.

Modifications

  • Check if "RestException" is a redirect code.
  • Clean up some unused methods.
  • Add some missing exceptionallyhandlers that will make the request timeout.

like code lines 3048 - 3427

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • no-need-doc

Q&A

Q: Why remove code as follow:

 if (cause instanceof WebApplicationException
                            && ((WebApplicationException) cause).getResponse().getStatus()
                            == Status.TEMPORARY_REDIRECT.getStatusCode()) {
                            if (log.isDebugEnabled()) {
                                log.debug("[{}] Failed to get subscriptions for non-partitioned topic {},"
                                                + " redirecting to other brokers.", clientAppId(), topicName, cause);
                            }
                    }

A: because some methods like validateTopicOwnershipAsync already have logged.

if (!isTopicOwned) {
boolean newAuthoritative = isLeaderBroker(pulsar());
// Replace the host and port of the current request and redirect
URI redirect = UriBuilder.fromUri(uri.getRequestUri())
.host(webUrl.getHost())
.port(webUrl.getPort())
.replaceQueryParam("authoritative", newAuthoritative)
.build();
// Redirect
if (log.isDebugEnabled()) {
log.debug("Redirecting the rest call to {}", redirect);
}
throw new WebApplicationException(Response.temporaryRedirect(redirect).build());

Additional

We have some code like this:

image

Multiple exception handlers will cause some code complexity, I think we can merge those handlers and refactor method in another PR.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 13, 2022
@codelipenghui codelipenghui added this to the 2.10.0 milestone Feb 14, 2022
@codelipenghui
Copy link
Contributor

@mattisonchao Please rebase the master branch

@hangc0276
Copy link
Contributor

@mattisonchao Thanks for your contribution, please help resolve the conflicts, thanks a lot.

@mattisonchao
Copy link
Member Author

@hangc0276 @Demogorgon314 @RobertIndie PTAL :)
Thanks.

@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codelipenghui codelipenghui merged commit c7030ab into apache:master Feb 14, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants