Github user hmcl commented on a diff in the pull request:
https://github.com/apache/storm/pull/2428#discussion_r155909342
--- Diff:
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java
---
@@ -359,7 +356,7 @@ private Builder(Builder<?, ?> builder,
SerializableDeserializer<K> keyDes, Class
*/
@Deprecated
public <NK> Builder<NK, V> setKey(Class<? extends
Deserializer<NK>> clazz) {
--- End diff --
@srdo do you know why this method returns a new builder object? I can't
figure a reason for it to so. I suspect that the only reason for that to happen
is because the fields of the builder class are final (e.g keyDesClassClazz),
and to make the generics work. There is no benefit in having fields inside the
builder class to be final. The code snippet bellow also fixes the generics
problem. Any reason not to get rid of the builder (with copy constructor) class
completely and make this method like this:
```java
public Builder<K,V> setKey(Class<? extends Deserializer<K>> clazz) {
this.keyDesClazz = clazz;
if (keyDesClazz != null) {
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDesClazz);
}
return this;
}
```
We should do something similar to the other 3 methods. In my opinion has
become a bit confusing, and I believe this is one of the last few opportunities
we have to make it better. Please let me know your thoughts. Thanks.
---
