Skip to content

Discovery service should use HTTP temporary redirect (307) #204

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
Feb 15, 2017

Conversation

merlimat
Copy link
Contributor

Motivation

HTTP redirections in discovery service component need to be done with 307 code instead of 302, as the broker is already doing.

When redirecting with 302, the HTTP method gets converted into a GET in the subsequent requests, when originally it might have been a POST or a PUT request.

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Feb 12, 2017
@merlimat merlimat added this to the 1.17 milestone Feb 12, 2017
@merlimat merlimat self-assigned this Feb 12, 2017
Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

👍

@rdhabalia
Copy link
Contributor

@merlimat this fix has caught a bug in test-case as well:

Failed tests: 
  DiscoveryServiceWebTest.testRiderectUrlWithServerStarted:99 expected [Cannot set replication on a non-global namespace] but found [Cannot get the replication clusters for a non-global namespace]

Can we change assert-string as : Cannot set replication on a non-global namespace

@merlimat
Copy link
Contributor Author

Yes, the test was sending a POST and comparing with the response for the GET operation. I'll fix that

@merlimat merlimat force-pushed the discovery-service-redirection branch 2 times, most recently from 5986c63 to 647490c Compare February 14, 2017 17:48
@saandrews
Copy link
Contributor

👍

@merlimat merlimat force-pushed the discovery-service-redirection branch from 647490c to ced33b5 Compare February 14, 2017 21:22
@merlimat merlimat force-pushed the discovery-service-redirection branch from ced33b5 to c928664 Compare February 15, 2017 20:30
@merlimat merlimat changed the title Directory service should use HTTP temporary redirect (307) Discovery service should use HTTP temporary redirect (307) Feb 15, 2017
@merlimat merlimat merged commit c890005 into apache:master Feb 15, 2017
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants