STORM-2548: Simplify KafkaSpoutConfig to avoid duplicating KafkaConsu… by srdo · Pull Request #2155 · apache/storm

srdo · 2017-06-10T10:53:26Z

…mer configuration parameters

This is put up for discussion, since a few people on the mailing list expressed a desire for a simpler KafkaSpoutConfig.

@harshach and @priyank5485 I think this is something like what you were asking for?

priyank5485 · 2017-06-12T21:22:46Z

+    }
+
+    private static Builder<String, String> setDefaultStringDeserializers(Builder<String, String> builder) {
+        builder.setProp(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, StringDeserializer.class);


I think ByteArrayDeserializer makes more sense. Not a strong preference though.

This is just here to support the 3 convenience builder() functions above. I'll rename it so it doesn't imply that String is the default type.

priyank5485 · 2017-06-12T21:26:00Z

@srdo Overall, this looks good. But I also think we should get rid of the Builder class totally. Reason is that it does not work well with flux yaml since you create the object using builder.build(). Let me know your thoughts on that.

srdo · 2017-06-13T16:38:18Z

(copying from the mailing list so it is easier to track the discussion if someone reads this later)

With regard to getting rid of the builder pattern, I think it is a pretty nice pattern for Java. It looks to me like it should be possible to declare and configure the builder with "component:", and then pass it to the KafkaSpoutConfig constructor with "ref:" after (which lets you avoid calling build()). Doesn't this work?

priyank5485 · 2017-06-21T18:22:16Z

+1(NB) given that people prefer keeping the Builder class around.

harshach · 2017-06-26T17:50:14Z

+1

hmcl · 2017-06-28T15:24:29Z

        UNCOMMITTED_LATEST }

+    /**
+     * Convenience method to get a Builder for String key/value spouts. Sets the key/value Deserializers to the StringDeserializer class.


NIT: I would suggest a more succinct javadoc, e.g. "Factory method that creates a Builder with key/val String deserializers.". In my opinion the text mentioning how to use builders for other ser/des should go in the README.

Same comment would apply to other Builder factory methods.

Sure, will shorten this

hmcl · 2017-06-28T15:50:50Z

-                Class<? extends Deserializer<K>> keyDesClazz,
-                SerializableDeserializer<V> valDes, Class<? extends Deserializer<V>> valDesClazz, Subscription subscription) {
+        /**
+         * Create a KafkaSpoutConfig builder.


Create a KafkaSpoutConfig builder with default property values. Properties can be overridden with the respective builder set methods.

hmcl · 2017-06-28T15:52:04Z

-
-        /**
-         * Set a Kafka property config.
+         * Set a Kafka consumer property config. 


Set a Kafka consumer property

hmcl · 2017-06-28T16:14:02Z


        /**
-         * Set multiple Kafka property configs.
+         * Set multiple Kafka consumer property configs.


Set multiple Kafka consumer properties

hmcl · 2017-06-28T16:14:21Z


        /**
-         * Set multiple Kafka property configs.
+         * Set multiple Kafka consumer property configs.


Set multiple Kafka consumer properties

hmcl · 2017-06-28T16:47:55Z

+         * Set multiple Kafka consumer property configs.
         */
        public Builder<K,V> setProp(Properties props) {
            for (String name: props.stringPropertyNames()) {


This code has one bug as it only picks properties for which both key and value are both strings. It should also pick key/value pairs that are String/Object. I suggest that we refactor this code to be as such:

public Builder<K,V> setProp(Properties props) { props.forEach((key, value) -> { if (key instanceof String) { kafkaProps.put((String) key, value); } }); return this; }

Nice catch. How about we use putAll to put the properties into kafkaProps instead? I'm not sure that I like the method filtering the input properties, I'd rather throw an error if the key is not a String. At the same time I don't think it should be KafkaSpoutConfig's job to validate KafkaConsumer parameters. The KafkaConsumer will handle throwing the error if the keys are not Strings https://github.com/apache/kafka/blob/efb060c57f05d1d586bb14c016b0187c60f8e994/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java#L60

Nevermind, that won't work since KafkaSpoutConfig enforces the String type on keys. I'd at least like to throw an exception if the input properties contain a non-String key.

srdo · 2017-06-28T18:11:40Z

@hmcl Addressed your comments, and updated the README to reflect the API changes. Also fixed some formatting and markdown issues.

hmcl · 2017-06-28T18:44:57Z

@srdo thanks for addressing the code comments. It LGTM, but I forgot to publish the following comment - sorry for the extra overhead. Do you want to address it in this patch as well ?

Most of the defaults in here are also in here. I think it would be better to have de defaults only in one place, ideally in the top level class. However, if it makes the code simpler to leave the defaults only in the Builder class, so be it. Neertheless, we should avoid having them in both places.

I also would like to ask if you can squash all the commits once we are done addressing everything, such that we can keep the git log clean. Thanks a lot.

srdo · 2017-06-28T18:57:51Z

@hmcl I'm not sure how we can do that. The fields in Builder are not static, so if we move the default definitions there, we'd have to create a builder and fish out the default values in the tests. The defaults also wouldn't be visible on the KafkaSpoutConfig class, which would probably be a minus. I don't see a way to have the defaults solely in the KafkaSpoutConfig class with no references to them from Builder, since the Builer fields have to get the default values if nothing else is set.

We could move the FirstPollOffsetStrategy default into a static field in KafkaSpoutConfig like the rest for consistency at least?

srdo · 2017-06-28T18:58:32Z

I'll squash the commits soon, I just wanted people to have a chance to review the changes without having to read the entire diff again

hmcl · 2017-06-28T19:01:26Z

Yeah... I also think that's the ideal way to do it. Squash at the end and have a new commit addressing each batch of code review comments.

hmcl · 2017-06-29T22:51:08Z

@srdo I agree with you concerning this comment. There is really not a much better way to do this. I agree with your comment "move the FirstPollOffsetStrategy default into a static field in KafkaSpoutConfig like the rest for consistency" - let's do it.

I think that what confused me is the way the whole class is organized. For instance, the state of KafkaSpoutConfig is not at the top of the class as one would expect. Perhaps we should follow the usual convention and move the constructor and fields to the top, after the constants, then put the static inner classes (here and here), then the factory methods, and at last all the accessor methods.

On a different note, I noticed that this PR now has the changes from the STORM-2541 PR included in its single commit. I think that this PR should have only one commit, with the refactoring of KafkaSpoutConfig. The STORM-2541 PR should have two commits, this PR's commit, and its own commit. At last the STORM-2542 PR should have three commits, this PR's commit, STORM-2541 PR commit, and its own commit.

That would make it easy to merge and the same time not loose Git history.

hmcl · 2017-06-29T23:09:12Z

@srdo Thanks for your diligence and awesome work refactoring this code. It just made it much better.

srdo · 2017-07-04T20:44:47Z

@hmcl Added a field for the offset strategy default and reordered the KafkaSpoutConfig contents as you suggested.

I didn't mean for the branches to contain each other, I accidentally reset a branch to the wrong content, should be fixed now. Once this or STORM-2541 goes in I'll fix any conflicts that pop up in the other PR.

srdo · 2017-07-04T20:56:21Z

Actually you're right, building them on each other would be faster. I'll do as you suggested

srdo · 2017-07-15T10:26:02Z

There were some +1s earlier, but this branch has changed a bunch since. I'd appreciate if someone could give it a skim and check that it's still good.

hmcl · 2017-07-17T16:57:57Z

+1. @harshach @priyank5485 can you please take one final look. If you don't have any objection, I suggest that we merge this patch in the next day or so.

harshach · 2017-07-17T20:58:18Z

still +1 .

…mer configuration parameters

srdo · 2017-07-18T16:47:45Z

Thanks for the reviews everyone.

srdo force-pushed the STORM-2548 branch 3 times, most recently from c980335 to e6ca97f Compare June 10, 2017 18:38

priyank5485 reviewed Jun 12, 2017

View reviewed changes

srdo force-pushed the STORM-2548 branch from e6ca97f to 73adaa9 Compare June 13, 2017 16:37

hmcl reviewed Jun 28, 2017

View reviewed changes

srdo force-pushed the STORM-2548 branch from 21e7e30 to 142d1f8 Compare June 28, 2017 20:28

This was referenced Jun 29, 2017

STORM-2541: Fix storm-kafka-client manual subscription not being able… #2150

Merged

STORM-2542: Remove KafkaConsumer.subscribe API option, make KafkaConsumer.assign the default #2151

Merged

srdo force-pushed the STORM-2548 branch from 974c94d to bd21c8e Compare July 4, 2017 20:52

srdo force-pushed the STORM-2548 branch from bd21c8e to ffe8cba Compare July 14, 2017 20:23

srdo mentioned this pull request Jul 15, 2017

STORM-2548: Simplify KafkaSpoutConfig (1.x) #2215

Merged

STORM-2548: Simplify KafkaSpoutConfig to avoid duplicating KafkaConsu…

01a56ec

…mer configuration parameters

srdo force-pushed the STORM-2548 branch from ffe8cba to 01a56ec Compare July 18, 2017 09:53

asfgit merged commit 01a56ec into apache:master Jul 18, 2017

srdo mentioned this pull request Nov 20, 2017

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

Merged

Uh oh!

Conversation

srdo commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyank5485 commented Jun 12, 2017

Uh oh!

srdo commented Jun 13, 2017

Uh oh!

priyank5485 commented Jun 21, 2017

Uh oh!

harshach commented Jun 26, 2017

Uh oh!

hmcl Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srdo commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmcl commented Jun 28, 2017

Uh oh!

srdo commented Jun 28, 2017

Uh oh!

srdo commented Jun 28, 2017

Uh oh!

hmcl commented Jun 28, 2017

Uh oh!

hmcl commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmcl commented Jun 29, 2017

Uh oh!

srdo commented Jul 4, 2017

Uh oh!

srdo commented Jul 4, 2017

Uh oh!

srdo commented Jul 15, 2017

Uh oh!

hmcl commented Jul 17, 2017

Uh oh!

harshach commented Jul 17, 2017

Uh oh!

srdo commented Jul 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hmcl Jun 28, 2017 •

edited

Loading

srdo commented Jun 28, 2017 •

edited

Loading

hmcl commented Jun 29, 2017 •

edited

Loading