[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14078: [Proxy] Fix port exhaustion and connection issues in Pulsar Proxy

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #14078:
URL: https://github.com/apache/pulsar/pull/14078#discussion_r796843931



##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
##########
@@ -481,6 +504,36 @@ public HAProxyMessage getHAProxyMessage() {
         return haProxyMessage;
     }
 
-    private static final Logger LOG = LoggerFactory.getLogger(ProxyConnection.class);
+    private boolean isBrokerActive(String targetBrokerHostPort) {
+        for (ServiceLookupData serviceLookupData : getAvailableBrokers()) {
+            if (matchesHostAndPort("pulsar://", serviceLookupData.getPulsarServiceUrl(), targetBrokerHostPort)
+                    || matchesHostAndPort("pulsar+ssl://", serviceLookupData.getPulsarServiceUrlTls(),
+                    targetBrokerHostPort)) {
+                return true;
+            }
+        }
+        return false;
+    }
 
+    private List<? extends ServiceLookupData> getAvailableBrokers() {
+        if (service.getDiscoveryProvider() == null) {
+            LOG.warn("Unable to retrieve active brokers. service.getDiscoveryProvider() is null."
+                    + "zookeeperServers and configurationStoreServers must be configured in proxy configuration "
+                    + "when checkActiveBrokers is enabled.");
+            return Collections.emptyList();
+        }

Review comment:
       +1 for failing fast on start up--that will make it clear to users that it is not configured properly.
   
   In k8s, it is easy to configure a service as headless, so that it resolves to a set of IP addresses https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#a-aaaa-records. I believe this is possible for all DNS providers, too. Would it be possible to add a feature to the proxy to consume the set of IPs to make the list of "available brokers" equivalent to the result of resolving the broker DNS entry?
   
   I see that https://pulsar.apache.org/docs/en/administration-proxy/#use-broker-urls already recommends using a DNS entry that points to all brokers. I haven't looked closely, but given that the helm chart is not configured to use a headless service, I assume that the proxy is not able to consume a set of IPs yet.
   
   Also, by using a headless service, we'd eliminate an additional network proxy hop.




-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org