You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/27 04:08:44 UTC
[GitHub] [kafka] ijuma commented on a change in pull request #8605: MINOR: align the constructor of KafkaConsumer to KafkaProducer
ijuma commented on a change in pull request #8605:
URL: https://github.com/apache/kafka/pull/8605#discussion_r430426504
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java
##########
@@ -573,18 +573,6 @@ private void maybeOverrideClientId(Map<String, Object> configs) {
return newConfigs;
}
- public static Properties addDeserializerToConfig(Properties properties,
Review comment:
We cannot remove this method since it's in a class that is part of Kafka's public API. We should probably deprecate it, but we need a KIP for that.
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
##########
@@ -480,18 +480,6 @@ private static String parseAcks(String acksString) {
return newConfigs;
}
- public static Properties addSerializerToConfig(Properties properties,
- Serializer<?> keySerializer,
- Serializer<?> valueSerializer) {
- Properties newProperties = new Properties();
- newProperties.putAll(properties);
- if (keySerializer != null)
- newProperties.put(KEY_SERIALIZER_CLASS_CONFIG, keySerializer.getClass().getName());
- if (valueSerializer != null)
- newProperties.put(VALUE_SERIALIZER_CLASS_CONFIG, valueSerializer.getClass().getName());
- return newProperties;
- }
-
Review comment:
We cannot remove this method since it's in a class that is part of Kafka's public API. We should probably deprecate it, but we need a KIP for that.
##########
File path: clients/src/main/java/org/apache/kafka/common/utils/Utils.java
##########
@@ -1184,4 +1185,22 @@ private static byte checkRange(final byte i) {
result.removeAll(right);
return result;
}
+
+ /**
+ * convert a properties to map. All keys in properties must be string type. Otherwise, a ConfigException is thrown.
Review comment:
Nit: capitalize please.
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
##########
@@ -480,18 +480,6 @@ private static String parseAcks(String acksString) {
return newConfigs;
}
- public static Properties addSerializerToConfig(Properties properties,
- Serializer<?> keySerializer,
- Serializer<?> valueSerializer) {
- Properties newProperties = new Properties();
- newProperties.putAll(properties);
- if (keySerializer != null)
- newProperties.put(KEY_SERIALIZER_CLASS_CONFIG, keySerializer.getClass().getName());
- if (valueSerializer != null)
- newProperties.put(VALUE_SERIALIZER_CLASS_CONFIG, valueSerializer.getClass().getName());
- return newProperties;
- }
-
Review comment:
I think you could submit a KIP for the deprecation of the two methods in this class, but we can merge the other changes in the meantime.
----------------------------------------------------------------
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