[GitHub] [kafka] kkonstantine commented on a change in pull request #10841: KAFKA-12482 Remove deprecated rest.host.name and rest.port configs

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

kkonstantine commented on a change in pull request #10841:
URL: https://github.com/apache/kafka/pull/10841#discussion_r655880156



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java
##########
@@ -497,6 +476,37 @@ static void validateHeaderConfigAction(String action) {
                     + "Expected one of %s", action, HEADER_ACTIONS));
         }
     }
+    private static class ListenersValidator implements ConfigDef.Validator {
+        @Override
+        public void ensureValid(String name, Object value) {
+            if (value == null) {
+                throw new ConfigException("Invalid value for listeners, at least one URL is expected, ex: http://localhost:8080,https://localhost:8443.");
+            }
+
+            if (!(value instanceof List)) {

Review comment:
       `instanceof` checks for `null` too. I wonder if it's better to combine these two cases to say that we expect a list with at least one value (meaning a non-empty list). 

##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java
##########
@@ -497,6 +476,37 @@ static void validateHeaderConfigAction(String action) {
                     + "Expected one of %s", action, HEADER_ACTIONS));
         }
     }
+    private static class ListenersValidator implements ConfigDef.Validator {
+        @Override
+        public void ensureValid(String name, Object value) {
+            if (value == null) {
+                throw new ConfigException("Invalid value for listeners, at least one URL is expected, ex: http://localhost:8080,https://localhost:8443.");
+            }
+
+            if (!(value instanceof List)) {
+                throw new ConfigException("Invalid value type for listeners (expected list).");
+            }
+
+            List items = (List) value;
+            if (items.isEmpty()) {
+                throw new ConfigException("Invalid value for listeners, at least one URL is expected, ex: http://localhost:8080,https://localhost:8443.");
+            }
+
+            for (Object item: items) {

Review comment:
       nit: I think formatting corrects this (at least on intellij). Can be fixed below too
   ```suggestion
               for (Object item : items) {
   ```

##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java
##########
@@ -497,6 +476,37 @@ static void validateHeaderConfigAction(String action) {
                     + "Expected one of %s", action, HEADER_ACTIONS));
         }
     }
+    private static class ListenersValidator implements ConfigDef.Validator {
+        @Override
+        public void ensureValid(String name, Object value) {
+            if (value == null) {
+                throw new ConfigException("Invalid value for listeners, at least one URL is expected, ex: http://localhost:8080,https://localhost:8443.");
+            }
+
+            if (!(value instanceof List)) {
+                throw new ConfigException("Invalid value type for listeners (expected list).");
+            }
+
+            List items = (List) value;

Review comment:
       Using generic types instead of raw types for collections is preferable (we can fix elsewhere in the file too)
   ```suggestion
               List<?> items = (List<?>) value;
   ```
   




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

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