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.

Sign up for GitHub

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

Jump to bottom

Allow user to configure proxy as reverse HTTP proxy #2801

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Allow user to configure proxy as reverse HTTP proxy #2801

merged 2 commits into from
Oct 17, 2018

Conversation

ivankelly
Copy link
Contributor

Users normally want to expose as few endpoints as possible. This patch
enables configuring the pulsar proxy as a review HTTP proxy, so that
it can be used as a single endpoint for all pulsar and even non-pulsar
HTTP services.

The reverse proxy uses jetty's built-in implementation.

It is configured in proxy.conf, in the form

httpReverseProxy.NAME.path = '/path-on-pulsar-proxy-endpoint'
httpReverseProxy.NAME.proxyTo = 'http://internal-server/some-app'

where NAME is any user-defined string. Multiple paths can be
configured this way.

@ivankelly
Allow user to configure proxy as reverse HTTP proxy
8176389 
Users normally want to expose as few endpoints as possible. This patch
enables configuring the pulsar proxy as a review http proxy, so that
it can be used as a single endpoint for all pulsar and even non-pulsar
HTTP services.

The reverse proxy uses jetty's builtin implementation.

It is configured in proxy.conf, in the form

httpReverseProxy.NAME.path = '/path-on-pulsar-proxy-endpoint'
httpReverseProxy.NAME.proxyTo = 'http://internal-server/some-app'

where NAME is any user defined string. Multiple paths can be
configured this way.
@ivankelly ivankelly added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/proxy labels Oct 17, 2018
@ivankelly ivankelly added this to the 2.3.0 milestone Oct 17, 2018
@ivankelly ivankelly self-assigned this Oct 17, 2018
@ivankelly
Copy link
Contributor Author

cc @cckellogg

merlimat
merlimat approved these changes Oct 17, 2018
View reviewed changes
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.

👍

@merlimat
Copy link
Contributor

run java8 tests
run integration tests

@merlimat
Copy link
Contributor

@ivankelly There was a failure in one of the new tests:

java.lang.AssertionError: expected [200] but found [504]
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)
	at org.testng.Assert.assertEqualsImpl(Assert.java:137)
	at org.testng.Assert.assertEquals(Assert.java:118)
	at org.testng.Assert.assertEquals(Assert.java:652)
	at org.testng.Assert.assertEquals(Assert.java:662)
	at org.apache.pulsar.proxy.server.ProxyIsAHttpProxyTest.testMultipleRedirect(ProxyIsAHttpProxyTest.java:167)

@ivankelly
Copy link
Contributor Author

@merlimat that looks very strange, as if one of the backing servers didn't come up. I've pushed another change which enabled jetty logging at info level which may shed more light. needless to say, i don't have a local repro.

@ivankelly
Copy link
Contributor Author

rerun integration tests

@merlimat merlimat merged commit 052c3b0 into apache:master Oct 17, 2018
@merlimat
Copy link
Contributor

@ivankelly One more failure here: https://builds.apache.org/job/pulsar_precommit_java8/4349/testReport/junit/org.apache.pulsar.proxy.server/ProxyIsAHttpProxyTest/

@ivankelly
Copy link
Contributor Author

looking into it

@ivankelly
Copy link
Contributor Author

@merlimat Looks like a legit issue. Seems to only repro if you have 24 cores though. There's a race somewhere, and I suspect it's in jetty itself

@merlimat
Copy link
Contributor

Ouch, maybe we can try to upgrade Jetty to latest stable too.

@ivankelly
Copy link
Contributor Author

I have a reliable repro, so will try the upgrade. I suspect it could be related to
jetty/jetty.project#2023

@ivankelly
Copy link
Contributor Author

@merlimat seems to happen even with the latest 9.3 series (from early sept). I need to put this down for the day now, but will pick it up again tomorrow.

@ivankelly
Copy link
Contributor Author

Created jetty/jetty.project#3005. Moving to 9.4 series, which seems to fix it.

ivankelly added a commit to ivankelly/pulsar that referenced this pull request Oct 19, 2018
@ivankelly
Bump jetty to v9.4.12
fda0654 
The recent proxy changes (apache#2801) tickled some issues in jetty's 9.3
series, where clients can hang if running on machines with a lot of
cores. This causes some tests to fail CI, while being impossible to
repro locally (unless you have a 24 core machine lying around).

Jetty reworked a lot of their selector code in 9.4, which appears to
make the issue go away.
@ivankelly ivankelly mentioned this pull request Oct 19, 2018
Merged
ivankelly added a commit that referenced this pull request Oct 22, 2018
@ivankelly
Bump jetty to v9.4.12 (#2811)
b73a967 
The recent proxy changes (#2801) tickled some issues in jetty's 9.3
series, where clients can hang if running on machines with a lot of
cores. This causes some tests to fail CI, while being impossible to
repro locally (unless you have a 24 core machine lying around).

Jetty reworked a lot of their selector code in 9.4, which appears to
make the issue go away.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@jai1 jai1 Awaiting requested review from jai1

@massakam massakam Awaiting requested review from massakam

Assignees

@ivankelly ivankelly

Labels
area/proxy type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivankelly @merlimat @sijie