[GitHub] [kafka] frosiere commented on a diff in pull request #12125: KAFKA-13864: provide the construct interceptor for KafkaProducer and KafkaConsumer

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

frosiere commented on code in PR #12125:
URL: https://github.com/apache/kafka/pull/12125#discussion_r866287593


##########
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##########
@@ -663,12 +663,38 @@ public KafkaConsumer(Properties properties,
     public KafkaConsumer(Map<String, Object> configs,
                          Deserializer<K> keyDeserializer,
                          Deserializer<V> valueDeserializer) {
+        this(configs, keyDeserializer, valueDeserializer, null);
+    }
+
+    /**
+     * A consumer is instantiated by providing a set of key-value pairs as configuration, and a key and a value {@link Deserializer}.
+     * <p>
+     * Valid configuration strings are documented at {@link ConsumerConfig}.
+     * <p>
+     * Note: after creating a {@code KafkaConsumer} you must always {@link #close()} it to avoid resource leaks.
+     *
+     * @param configs The consumer configs
+     * @param keyDeserializer The deserializer for key that implements {@link Deserializer}. The configure() method
+     *            won't be called in the consumer when the deserializer is passed in directly.
+     * @param valueDeserializer The deserializer for value that implements {@link Deserializer}. The configure() method
+     *            won't be called in the consumer when the deserializer is passed in directly.
+     * @param interceptors The list interceptors for consumer that implements {$link ConsumerInterceptor}.
+     */
+    public KafkaConsumer(Map<String, Object> configs,

Review Comment:
   To avoid touching to the existing and offer more flexibility, I would have created a new constructor with a ProducerConfig to allow overriding the method ProducerConfig#getConfiguredInstances to complete the list of interceptors with instances or any other type of instances such as the MetricsReporter of the partitioner. See the Spring issue for a concrete example: https://github.com/spring-projects/spring-kafka/issues/2244 
   This approach can work as well.



##########
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##########
@@ -663,12 +663,38 @@ public KafkaConsumer(Properties properties,
     public KafkaConsumer(Map<String, Object> configs,
                          Deserializer<K> keyDeserializer,
                          Deserializer<V> valueDeserializer) {
+        this(configs, keyDeserializer, valueDeserializer, null);
+    }
+
+    /**
+     * A consumer is instantiated by providing a set of key-value pairs as configuration, and a key and a value {@link Deserializer}.

Review Comment:
   the new parameter is not mentioned



##########
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:
##########
@@ -290,6 +290,27 @@ public KafkaProducer(Map<String, Object> configs, Serializer<K> keySerializer, S
                 keySerializer, valueSerializer, null, null, null, Time.SYSTEM);
     }
 
+    /**
+     * A producer is instantiated by providing a set of key-value pairs as configuration, a key and a value {@link Serializer}.

Review Comment:
   Same issue with the Javadoc of this constructor



##########
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##########
@@ -663,12 +663,38 @@ public KafkaConsumer(Properties properties,
     public KafkaConsumer(Map<String, Object> configs,
                          Deserializer<K> keyDeserializer,
                          Deserializer<V> valueDeserializer) {
+        this(configs, keyDeserializer, valueDeserializer, null);
+    }
+
+    /**
+     * A consumer is instantiated by providing a set of key-value pairs as configuration, and a key and a value {@link Deserializer}.
+     * <p>
+     * Valid configuration strings are documented at {@link ConsumerConfig}.
+     * <p>
+     * Note: after creating a {@code KafkaConsumer} you must always {@link #close()} it to avoid resource leaks.
+     *
+     * @param configs The consumer configs
+     * @param keyDeserializer The deserializer for key that implements {@link Deserializer}. The configure() method
+     *            won't be called in the consumer when the deserializer is passed in directly.
+     * @param valueDeserializer The deserializer for value that implements {@link Deserializer}. The configure() method
+     *            won't be called in the consumer when the deserializer is passed in directly.
+     * @param interceptors The list interceptors for consumer that implements {$link ConsumerInterceptor}.

Review Comment:
   Java doc is not correct. The list of interceptors... $link should be replaced by @link



-- 
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: jira-unsubscribe@kafka.apache.org

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