You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2017/11/19 15:43:49 UTC

[GitHub] storm pull request #2428: STORM-2826: Set key/value deserializer fields when...

GitHub user srdo opened a pull request:

    https://github.com/apache/storm/pull/2428

     STORM-2826: Set key/value deserializer fields when using the convenience builder methods in KafkaSpoutConfig

    Builds on STORM-2825.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/srdo/storm STORM-2826

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/storm/pull/2428.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2428
    
----
commit 299e92f39187d65bd9f9a900710a57429a84dec2
Author: Stig Rohde Døssing <sr...@apache.org>
Date:   2017-11-19T09:36:37Z

    STORM-2825: Fix ClassCastException when storm-kafka-client uses consumer config with String-type 'enable.auto.commit'

commit 8e082347bb25216bd18382ce52a516276bcc14d0
Author: Stig Rohde Døssing <sr...@apache.org>
Date:   2017-11-19T15:42:33Z

    STORM-2826: Set key/value deserializer fields when using the convenience builder methods in KafkaSpoutConfig

----


---

[GitHub] storm pull request #2428: STORM-2826: Set key/value deserializer fields when...

Posted by srdo <gi...@git.apache.org>.

Github user srdo commented on a diff in the pull request:

https://github.com/apache/storm/pull/2428#discussion_r153035015

--- Diff: external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java ---
@@ -320,20 +320,17 @@ private Builder(Builder<?, ?> builder, SerializableDeserializer<K> keyDes, Class
// when they change the key/value types.
this.translator = (RecordTranslator<K, V>) builder.translator;
this.retryService = builder.retryService;
-
- if (keyDesClazz != null) {
- this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDesClazz);
- }
+
if (keyDes != null) {
this.kafkaProps.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, keyDes.getClass());
- }
- if (valueDesClazz != null) {
- this.kafkaProps.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, valueDesClazz);
- }
- if (valueDes != null) {
+ } else if (keyDesClazz != null) {
--- End diff --

One of the changes in https://github.com/apache/storm/pull/2215 is to make users set most Kafka properties through setProp instead of duplicating configuration parameters in KafkaSpoutConfig. These properties include key and value deserializers.

In order to provide backward compatibility that PR tries to ensure that if the keyDes/keyDesClazz field is set, then the corresponding kafkaProps property is also set. The spout doesn't use the fields anymore, it only reads from the kafkaProps map. The setKey/setValue methods were also deprecated, and users were directed to use setProp instead. Additionally the convenience builder methods were changed so they didn't set the keyDes/keyDesClazz fields, but just set properties in kafkaProps instead.

The issue Alexandre hit during testing was that he was doing something like `kafkaSpoutConfig.getKeyDeserializer().getClass()` on a KafkaSpoutConfig built from one of the convenience builders. Since keyDes/keyDesClazz aren't being set by the default builders anymore, this causes an NPE.

I think we still want to be able to provide the simplified KafkaSpoutConfig API to 1.x, and we still want to encourage users to use setProp instead of setKey/setValue. In order to fix the NPE we also have to set keyDes/keyDesClazz in the convenience builder methods again.

Without this change the code behaves in a very confusing way when mixing use of setKey/setValue and setProp. The added unit tests demonstrate cases where the current code would act counterintuitively. Try pasting the current code into the modified Builder constructor, and you'll see test failures.

The modified constructor is used when setKey/setValue is called. With the current code, calling e.g. setKey will actually overwrite both the key and value properties in kafkaProps if keyDesClazz and valueDesClazz were set in the old Builder.

For example, if you did `KafkaSpoutConfig.builder(topic).setProp(MyValueDeserializer.class).setKey(MyKeyDeserializer.class)`, you end up with a KafkaSpoutConfig where the value deserializer is `StringDeserializer`. This is because the `builder` method now sets both keyDesClazz and valueDesClazz by default. When setKey calls the copy constructor, it overwrites both the key and value deserializer properties in kafkaProps with the values of the key/valueDesClazz fields. The updated code makes sure that if you call setKey, then only the key deserializer property will be set.

The change isn't necessary for the constructor further up, because it isn't a copy constructor. The problem with the copy constructor is that it takes in a kafkaProps that already might contain settings for key/value deserializers and overwrites them. The unmodified constructor has an empty kafkaProps, so there's no issue.

---