Closing client when Proxyconnection is disconnected - #1821
Conversation
sijie
left a comment
sijie
left a comment
There was a problem hiding this comment.
a simple unit test might be great.
| @@ -0,0 +1,40 @@ | |||
| package org.apache.pulsar.proxy.server; | |||
There was a problem hiding this comment.
There is no license header.
There was a problem hiding this comment.
Good catch - I will fix it
|
|
||
| if (client != null) { | ||
| client.close(); | ||
| } |
There was a problem hiding this comment.
I don't understand why we want to close the entire client at this point.
When this connection gets closed, we want its paired connection to be closed as well. This should already done at line 140 here above.
There was a problem hiding this comment.
The client connection Pool is used by lookupProxyHandler and not the directProxyHandler.
There was a problem hiding this comment.
But the client is shared across all the ProxyConnection instances
There was a problem hiding this comment.
We changed the logic to have one client per ProxyConnection
#1707
There was a problem hiding this comment.
Uhm, that doesn't sound a good option to me
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
@jai1 there are many resources that are created per-client, I don't think it's a working approach to have 1 client instance per proxy connection. |
|
I understand the resource aspect of it and agree that instead of having one client we can extract the connectionPool part out of the client and use one connection pool per proxy connection But we still need one connection pool per proxy connection since the broker authenticates the client and the proxy if So if Client A and Client B connect to Proxy we need one connection pool for A (since its A's credentials are authorized during handleConnect) and one connection pool for B. If we are in agreement let me know and I will start the work of having one connection pool instead of one client per proxy connection. |
I think the term "connection pool" is actually misleading. Proxy uses a 1-1 connections matching between (client -> proxy) and (proxy -> broker). No pooling is required, but rather just opening new connection each time. |
|
Ohk - now I understand the confusion The connection pool is used for lookups and getPartitionMetaData only once the broker discovery is done we use the 1 to 1 connection as you said. |
|
If this is blocking the release then we can defer it to the next one, else I will fix the tests and rebase the PR by Wednesday |
|
@jai1 great! Wednesday is good. |
|
@sijie - Can you review this PR so that we can merge it |
|
We can merge this now, though we should try to revisit the changes in #1707 for 2.2 |
4 participants
Currently, client.close() is not called when ProxyConnection becomes inactive which leads to leaking unused connections.
Since the ConnectionPool used in proxy uses the same event worker group as the Proxy service we can't close the event workgroup when we close the client, hence overriding the close() function in ProxyConnectionPool to just close the connections and not the event worker group.