[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13390: [security] Allow config client dns bind addr and port

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

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



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
##########
@@ -295,6 +295,19 @@
     @JsonIgnore
     private Clock clock = Clock.systemDefaultZone();
 
+    @ApiModelProperty(
+            name = "dnsLookupBindAddress",
+            value = "The Pulsar client dns lookup bind address."
+    )
+    private String dnsLookupBindAddress = null;
+
+    @ApiModelProperty(
+            name = "dnsLookupBindPort",
+            value = "The Pulsar client dns lookup bind port, takes effect when dnsLookupBindAddress is configured." +
+                    " default value is 0."
+    )
+    private int dnsLookupBindPort = 0;

Review comment:
       What does it mean to default to 0 here? From what I can tell, the `InetSocketAddress` will throw an `IllegalArgumentException` when the port is 0.
   
   ```java
       private static int checkPort(int port) {
           if (port < 0 || port > 0xFFFF)
               throw new IllegalArgumentException("port out of range:" + port);
           return port;
       }
   ```

##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
##########
@@ -295,6 +295,19 @@
     @JsonIgnore
     private Clock clock = Clock.systemDefaultZone();
 
+    @ApiModelProperty(
+            name = "dnsLookupBindAddress",
+            value = "The Pulsar client dns lookup bind address."

Review comment:
       Can you add the default value and that it means the client will bind to `0.0.0.0`?

##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
##########
@@ -342,14 +355,6 @@ public long getLookupTimeoutMs() {
         }
     }
 
-    public ClientConfigurationData clone() {
-        try {
-            return (ClientConfigurationData) super.clone();
-        } catch (CloneNotSupportedException e) {
-            throw new RuntimeException("Failed to clone ClientConfigurationData");
-        }
-    }
-

Review comment:
       I think we should keep this method where it is. We don't have a style guide covering this situation (that I know of), so I think we should keep things as they are until something more formal is decided on the mailing list.




-- 
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