lhotari commented on issue #10816:
URL: https://github.com/apache/pulsar/issues/10816#issuecomment-1378355110

   > You're referring to the final process, not lookup process, there are two 
connections between `connect to broker` and `lookup`.
   
   @nodece  I'm not exactly sure what you mean with this comment. 
   
   The [ProxyConnection is one of these 
states](https://github.com/apache/pulsar/blob/d6fcdb8e1e192e0d9e2fbe6307d273b27ed2930f/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java#L119-L141):
   ```java
     enum State {
           Init,
   
           // Connecting between user client and proxy server.
           // Mutual authn needs verify between client and proxy server several 
times.
           Connecting,
   
           // Proxy the lookup requests to a random broker
           // Follow redirects
           ProxyLookupRequests,
   
           // Connecting to the broker
           ProxyConnectingToBroker,
   
           // If we are proxying a connection to a specific broker, we
           // are just forwarding data between the 2 connections, without
           // looking into it
           ProxyConnectionToBroker,
   
           Closing,
   
           Closed,
       }
   ```
   
   Yes, there's a separate connection for lookups and after the state goes to 
ProxyConnectingToBroker/ProxyConnectionToBroker, the connection(s) used for 
lookups, will no longer be needed.
   
   This is what I meant with my comment "When the proxy switches to the mode 
where plain bytes are proxies, the proxy should never use the authentication 
data anymore. This is why it's unnecessary to keep it up-to-date after the 
proxy switches the connection to proxying mode."
   
   I see that you know this, but my point is that it's not described in the PR 
descriptions.
   It's really hard to guess what the intention is if there's no code comments 
or description in the PR of the intent. 
   
   For example, iterating through the connections in the connection pool (used 
for lookup connections) doesn't seem like the correct approach at all: 
https://github.com/apache/pulsar/pull/17831/files#diff-bbca5aac9dede7618187e91b91ffd0c6c8ffb836cd79ff2d104439e8cf5fc0daR545-R561
   
   Perhaps this works for most cases where there's a single connection in the 
connection pool. What if there are multiple connections? It would also be 
helpful to add integration tests where this would happen.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to