-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 ServiceUrlProvider and add method forceCloseConnection in PulsarC… #2543
Conversation
* | ||
* @param client created pulsar client. | ||
*/ | ||
void setClient(PulsarClient client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codelipenghui do you mind providing an example how this would be used?
@@ -706,6 +706,58 @@ public void shutdown() throws PulsarClientException { | |||
} | |||
} | |||
|
|||
@Override | |||
public void forceCloseConnection() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you describe more on how forceCloseConnection will be used in ServiceUrlProvider ?
@@ -706,6 +706,58 @@ public void shutdown() throws PulsarClientException { | |||
} | |||
} | |||
|
|||
@Override | |||
public void forceCloseConnection() { | |||
if (this.producers != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few problems with this way of closing the connections.
channelInactive()
is supposed to be called by Netty when the connection gets closed. With the current approache the original TCP connection will stay open and the broker will still have registered the producer/consumer as connected.
The right approach here should be to close all the connections in the ConnectionPool
. That will be triggering the reconnections for all producers/consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merlimat I'm already fix it.
run java8 tests |
@merlimat can you review this PR? |
retest this please |
run java8 tests |
over all lgtm, it would be better provide more comments and examples as @sijie suggested. |
run java8 tests |
Thanks @sijie. We will implement a service url provider base on Zookeeper. If this url provider have a good practice in zhaopin.com. we pleasure to contribute a Pulsar Client extend. |
@codelipenghui yeah, thank you so much for your contribution! look forward to your contribution about zookeeper based service url provider :) |
Support build Pulsar client with serviceUrlProvider method.
Motivation
With serviceUrlProvider we can store the pulsar service url in zookeeper or any other config service.
And we can watch the service url change event then control the pulsar client, such as change pulsar client serviceUrl, force close client connection or re-connect with new service url.
Modifications
Add ServiceUrlProvider interface.
Add forceCloseConnection method in PulsarClient.