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

Add Proxy support #37

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Add Proxy support #37

merged 1 commit into from
Jun 10, 2020

Conversation

mhelleborg
Copy link
Contributor

@mhelleborg mhelleborg commented Apr 15, 2020

This adds support for connection through a pulsar proxy. Tested against Kafkaeseque's SAAS offering.

This closes Issue 36

@blankensteiner
Copy link
Contributor

@mhelleborg Thanks! Will have a look at it later today! :-)

src/DotPulsar/Internal/ConnectionPool.cs Show resolved Hide resolved
src/DotPulsar/Internal/ConnectionPool.cs Show resolved Hide resolved
src/DotPulsar/Internal/ConnectionPool.cs Outdated Show resolved Hide resolved
src/DotPulsar/Internal/ConnectionPool.cs Outdated Show resolved Hide resolved
src/DotPulsar/Internal/ConnectionPool.cs Outdated Show resolved Hide resolved
@blankensteiner
Copy link
Contributor

Hi @mhelleborg
Thanks for the PR! :-) I have a few notes, let me know that you think.
Btw, have you had the chance to test this on a multi-broker-non-proxy-cluster?

@mhelleborg
Copy link
Contributor Author

Hi @mhelleborg
Thanks for the PR! :-) I have a few notes, let me know that you think.
Btw, have you had the chance to test this on a multi-broker-non-proxy-cluster?

I have not had the chance to test it on anything else yet, so only half-way verified

@blankensteiner
Copy link
Contributor

Hi @mhelleborg
Roger that, I'll find some time to test it before merging :-)

@blankensteiner
Copy link
Contributor

@mhelleborg
I have created some tests for a staging cluster where I work and sadly these changes break when connecting directly to brokers. Haven't got the time to dive into why at this point.

@mhelleborg
Copy link
Contributor Author

@mhelleborg
I have created some tests for a staging cluster where I work and sadly these changes break when connecting directly to brokers. Haven't got the time to dive into why at this point.

I don't have a non-proxy cluster here, so it will be hard to test. Can have a second look at the Java implementation to see if there is anything obvious.

@mhelleborg
Copy link
Contributor Author

@blankensteiner Seems to match the Java version, can you see what is failing against your cluster?

@blankensteiner
Copy link
Contributor

Hi @mhelleborg
I haven't had the time yet but will try and find some time next week.

@blankensteiner
Copy link
Contributor

Hi @mhelleborg
Our Pulsar test-cluster is down and I don't have the time to get it running again.
Maybe @janpieterz can assist with a non-proxy cluster? Or @sijie can assist with providing a docker-compose or Kubernetes YAML file that can spin up a multi-broker and multi-proxy cluster?
Anyway, in regards to the token fix, if you create that on another PR, I can go ahead and merge that :-)

@mhelleborg
Copy link
Contributor Author

@blankensteiner Created the separate PR.

I dont have time to debug this right now unfortunately, perhaps later this week.

@franck-schmidlin
Copy link

This PR works for me and connecting to kafkaesque. will see if I can spin up a compose to test the non-proxy aspect of it. no promises, I am new at all this.

@JarrodJ83
Copy link

Any update on this? Just curious as I am in need of this functionality as well.

@blankensteiner
Copy link
Contributor

Hi @JarrodJ83
Sadly we are still missing a PR for proxy support that doesn't break connecting to brokers.

@mhelleborg
Copy link
Contributor Author

@JarrodJ83 This has not been a priority recently, but I might be able to look at it this week. No promises :)

@JarrodJ83
Copy link

Thanks for the updates! @mhelleborg that would be very awesome! We are looking to leverage pulsar soon and we won't have direct access to the brokers so I'm really looking forward to this PR being merged. I'm not really sure what i could do to help as this is not an area of expertise of mine, but if there is anything, please let me know.

@mhelleborg
Copy link
Contributor Author

@blankensteiner What was the setup which failed when you tested? Seemed to work OK on the cases I tested locally, would be nice to have a repro :)

@blankensteiner
Copy link
Contributor

Hi @mhelleborg
It's a staging cluster consisting of 3 brokers.
Have you tried connecting to a multi-broker cluster on all brokers?

@mhelleborg
Copy link
Contributor Author

@blankensteiner Just direct connection to a single broker, as well as connecting to a cluster with Pulsar Proxy. Tested a compose for local cluster setup, but the one I found created an unhealthy cluster. Do you have a hot tip for a good compose setup for testing clusters locally?

@blankensteiner
Copy link
Contributor

Hi @mhelleborg
I haven't got a docker compose that will spin up a multi-broker and proxied cluster, but that would be cool though.
However, I think I've found the problem with the PR.

return _serviceUrl.IsLoopback ? connection : await GetConnection(physicalUrl, cancellationToken).ConfigureAwait(false);

Here we are using 'physicalUrl' but should actually GetBrokerServiceUrl from the LookupTopicReponse.
If you could fix that, test and rebase, then I can merge it and create a new release :-)

@franck-schmidlin
Copy link

NB: to test a cluster fronted by a proxy you can use kafkaesque free tier.
Hope this helps

@mhelleborg
Copy link
Contributor Author

@blankensteiner Thanks, will fix it when I have time later today :).

@franck-schmidlin Its already tested against Kafkaesque, works fine ;)

@mhelleborg
Copy link
Contributor Author

Did some minor changes, have a look @blankensteiner :)

@blankensteiner blankensteiner changed the title Add Proxy support, Token serialization -> UTF8 Add Proxy support Jun 10, 2020
@blankensteiner blankensteiner merged commit c8f10fe into apache:master Jun 10, 2020
@blankensteiner
Copy link
Contributor

Thanks for the PR, I appreciate it! :-)

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.

Support - Proxy
4 participants