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

Fix REST APIs provided by Pulsar proxy #1862

Merged
merged 5 commits into from
May 31, 2018
Merged

Conversation

massakam
Copy link
Contributor

@massakam massakam commented May 30, 2018

Motivation

The following entry points provided by Pulsar proxy does not work properly.

/status.html
/admin/*
/lookup/*

They always return an 404 error.

Modifications

Added clusterName to the proxy configuration in order to acquire a URL for proxying the http requests from configuration store servers.

Modified AdminProxyHandler to get the list of active brokers from ZooKeeper and route HTTP request to one of them in round-robin.

If brokerWebServiceURL or brokerWebServiceURLTLS is specified in proxy.conf, proxy always routes request to that URL.

The value of the existing brokerServiceURL can not be used, because it should be a URL that starts with pulsar://, not http://.

Also, modified WebServer to set the context path when adding proxy servlet.
Otherwise, the requests to /status.html are not handled properly by VipStatus.

Result

Pulsar proxy will be able to handle the http requests to the above entry points successfully.

@massakam massakam added the type/bug The PR fixed a bug or issue reported a bug label May 30, 2018
@massakam massakam added this to the 2.1.0-incubating milestone May 30, 2018
@massakam massakam self-assigned this May 30, 2018
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

One problem that still remains is when the service url is available.

This might be a common case when the proxy is indeed behind a load balancer (and thus have a single entry-point), but the brokers are not.

For Pulsar wire protocol, the proxy works fine since it gets the list of available brokers from ZK and it creates new connection in round-robin (unless it's targeting a specific broker).

For HTTP connection we should be doing the same, otherwise it would only work if the brokers are reachable through a load balancer.

An additional problem might also be that even though the brokers do have load balancer front, this might not be reachable from within the same network (or VPC), but only from the outside network. If the proxy is deployed in same network, it might not be able to connect to proxy.

Finally, I think we should fix this issue for 2.0.1 patch release, since right now the proxy would fail to start if the broker url is not set in proxy.conf

@massakam
Copy link
Contributor Author

Uhm... it makes sense.
I will reconsider the way to solve this problem.

@massakam massakam changed the title Fix REST APIs provided by Pulsar proxy [WIP] Fix REST APIs provided by Pulsar proxy May 31, 2018
@massakam massakam changed the title [WIP] Fix REST APIs provided by Pulsar proxy Fix REST APIs provided by Pulsar proxy May 31, 2018
@massakam
Copy link
Contributor Author

@merlimat Modified AdminProxyHandler to handle HTTP connection in the same way as Pulsar wire protocol. Would you review again?

@massakam
Copy link
Contributor Author

Finally, I think we should fix this issue for 2.0.1 patch release, since right now the proxy would fail to start if the broker url is not set in proxy.conf

Changed the milestone to 2.0.1.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍 Thanks @massakam

@merlimat merlimat merged commit 13ad7a6 into apache:master May 31, 2018
merlimat pushed a commit to merlimat/pulsar that referenced this pull request Jun 1, 2018
* Fix REST APIs provided by Pulsar proxy

* Rename status file for test

* Exclude status file from license check

* Add license header to status file

* Proxying HTTP request in the same way as Pulsar wire protocol
@merlimat
Copy link
Contributor

merlimat commented Jun 1, 2018

Backported to branch-2.0 at afa4e64

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.

None yet

5 participants