Skip to content

Fix 307 redirect errors when connecting to multi-broker cluster #503

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

Merged
merged 1 commit into from
Dec 30, 2022

Conversation

Apurva007
Copy link
Contributor

Fixes #274

Motivation

This change fixes the invalid redirect errors when the pulsar manager tries to connect to a multi broker cluster.

Modifications

Added a custom redirect strategy in the Http
Client utilized by Zuul. This will make sure that POST/DELETE/PUT methods will also be redirected in addition to the default GET/HEAD methods.

Verifying this change

Screen Shot 2022-12-29 at 1 51 27 AM

Screen Shot 2022-12-29 at 1 48 09 AM

  • [ x] Make sure that the change passes the ./gradlew build checks.

@Apurva007
Copy link
Contributor Author

@tuteng @nodece @rdhabalia Please can you review this change.

};
@Override
protected boolean isRedirectable(final String method) {
for (final String m: redirectHttpMethods) {
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 we can return true directly, don't check the HTTP methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nodece Thanks for the review. I have made the suggested changes. Please can you review it again.

import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpHead;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
Copy link
Member

Choose a reason for hiding this comment

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

Remove unused imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nodece Thanks for pointing it out. Removed the unused imports.

@nodece nodece requested a review from tuteng December 30, 2022 02:44
@nodece nodece merged commit d92cfd8 into apache:master Dec 30, 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.

Handle broker 307 redirect
2 participants