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

Cancel keep-alive timer task after the proxy switch to TCP proxy #1210

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Feb 9, 2018

Motivation

After initial handshake, the Pulsar proxy switches to TCP proxy mode, by just copying buffers between the 2 connections and avoiding all parsing.

When that happens, for keep-alive messages, we rely on what the client and broker are exchanging, so they will be able to detect a stale connection client-proxy or proxy-broker.
Currently, we're not removing the keep-alive timer in the proxy, so it's forcefully closing the connection after 60s, even though client and broker are perfectly fine.

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Feb 9, 2018
@merlimat merlimat added this to the 1.22.0-incubating milestone Feb 9, 2018
@merlimat merlimat self-assigned this Feb 9, 2018
@rdhabalia
Copy link
Contributor

so it's forcefully closing the connection after 60s

but right now, shouldn't proxy also handle client's keep-alive request on PulsarHandler.handlePing() which should keep connection alive between client to proxy and same way proxy to broker?

@merlimat
Copy link
Contributor Author

merlimat commented Feb 9, 2018

but right now, shouldn't proxy also handle client's keep-alive request on PulsarHandler.handlePing() which should keep connection alive between client to proxy and same way proxy to broker?

The proxy just passes along bytes at that point. The ping from client is just passed on to broker, which is the one that will respond. If there is a network partition either between client/proxy or proxy/broker, it will be identfied.

@rdhabalia
Copy link
Contributor

yes, once proxy state is in ProxyConnectionToBroker then it becomes just pass through.

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.

👍

@merlimat merlimat merged commit f288db5 into apache:master Feb 9, 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.

None yet

2 participants