STORM-2225: change spout config to be simpler. (1.x) #1868

revans2 · 2017-01-09T17:00:32Z

No description provided.

revans2 · 2017-01-09T17:00:52Z

This is the 1.x version of #1808

revans2 · 2017-01-11T21:44:28Z

The test failures are unrelated and are around the integration tests that always seem to fail lately.

ppoulosk · 2017-01-11T21:58:37Z

...les/src/main/java/org/apache/storm/kafka/trident/TridentKafkaClientWordCountNamedTopics.java

+	};
+
+    protected KafkaSpoutConfig<String,String> newKafkaSpoutConfig() {
+        return KafkaSpoutConfig.builder("127.0.0.1:9092", TOPIC_1, TOPIC_2)


Why are we using a hardcoded port here?

This is an exact translation of the original code. Even down to not using KAFKA_LOCAL_BROKER. If people want me to change it I am happy to, but I thought it best to not overreach on the scope of the pull request. At least until the code worked.

ppoulosk · 2017-01-11T22:45:12Z

...kafka-client/src/main/java/org/apache/storm/kafka/bolt/selector/FieldIndexTopicSelector.java

+
+    @Override
+    public String getTopic(Tuple tuple) {
+        if (fieldIndex < tuple.size()) {


Protect against negative index?

harshach · 2017-01-14T05:25:50Z

external/storm-kafka-client/README.md

@@ -1,192 +1,5 @@
-#Storm Kafka Spout with New Kafka Consumer API
+#Storm Kafka integration using the kafka-client jar


can we call it as Kafka New consumer API. Not many understand its using newer api by calling it as "kafka-client" jar

harshach · 2017-01-14T05:36:26Z

...l/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/StormStringDeserializer.java

-    public KafkaSpoutTuplesBuilderWildcardTopics(KafkaSpoutTupleBuilder<K, V> tupleBuilder) {
-        this.tupleBuilder = tupleBuilder;
-    }
+public class StormStringDeserializer extends StringDeserializer implements SerializableDeserializer<String> {


Do we still need this? users can configure the StringSerializer from kafka-clients lib

Yes we need this even more now. The Kafka Deserializer (including StringDeserializer) is not java serializable. So if we don't do this on a real storm cluster we will get exceptions when we try to write out the spout.

I can look into trying to support some kind of generics like

public <NK> Builder<NK,V> setKey(Class<? extends Deserializer<NK>> keyDeserializer) {

But I really don't know if that works. I'll try it out and let you know.

harshach · 2017-01-14T05:37:58Z

@revans2 overall looks good to me. Minor nits.

ppoulosk · 2017-01-17T14:56:42Z

+1, Non-binding.

revans2 · 2017-01-17T18:47:00Z

@harshach and @ppoulosk I just pushed fixes for your review comments. @harshach you were right I could and did remove StormStringDeserializer. I think this made the code much better.

ppoulosk · 2017-01-17T18:49:45Z

Thanks, @revans2. Still +1, NB.

harshach · 2017-01-24T01:38:25Z

+1. @revans2 can you squash the commits.

revans2 · 2017-01-24T15:14:10Z

@harshach I am happy to once I get a +1 on #1808 too. It has almost identical code, but is for master.

into STORM-2236-1.x-merge" This reverts commit 389966e, reversing changes made to 3979777.

STORM-2225: change spout config to be simpler. STORM-2228: removed ability to request a single topic go to multiple streams STORM-2236: Reimplemented manual partition management on top of STORM-2225

revans2 · 2017-01-30T21:13:23Z

This branch is also in sync with #1808 now tabs are removed and code is squashed.

hmcl · 2017-01-30T21:41:21Z

+1

HeartSaVioR · 2017-01-31T00:53:21Z

@revans2
Seems like the commit for JDK 7 is removed so Travis CI shows compile error on JDK 7. Could you address this?

revans2 · 2017-01-31T14:13:36Z

It was two jdk8 annotations that I didn't remove as part of rework/upmerging. I'll get them

revans2 · 2017-01-31T14:36:40Z

@HeartSaVioR I removed the unneeded annotation and rebuilt with JDK7

HeartSaVioR · 2017-02-01T05:13:13Z

+1

revans2 mentioned this pull request Jan 9, 2017

STORM-2225: change spout config to be simpler. #1808

Merged

ppoulosk reviewed Jan 11, 2017

View reviewed changes

harshach reviewed Jan 14, 2017

View reviewed changes

revans2 force-pushed the STORM-2225-1.x branch 2 times, most recently from 17a2e6c to 094d6dc Compare January 23, 2017 21:12

Robert (Bobby) Evans added 2 commits January 30, 2017 14:47

Revert "Merge branch 'STORM-2236-1.x' of https://github.com/MediaV/storm

cbffc00

into STORM-2236-1.x-merge" This reverts commit 389966e, reversing changes made to 3979777.

STORM-1997: copy state/bolt from storm-kafka to storm-kafka-client

a3e6f60

STORM-2225: change spout config to be simpler. STORM-2228: removed ability to request a single topic go to multiple streams STORM-2236: Reimplemented manual partition management on top of STORM-2225

revans2 force-pushed the STORM-2225-1.x branch from 094d6dc to a3e6f60 Compare January 30, 2017 21:11

Missed one more JDK7 issue

2986d0c

asfgit merged commit 2986d0c into apache:1.x-branch Feb 1, 2017

		@@ -1,192 +1,5 @@
		#Storm Kafka Spout with New Kafka Consumer API
		#Storm Kafka integration using the kafka-client jar

STORM-2225: change spout config to be simpler. (1.x) #1868

STORM-2225: change spout config to be simpler. (1.x) #1868

Uh oh!

Conversation

revans2 commented Jan 9, 2017

Uh oh!

revans2 commented Jan 9, 2017

Uh oh!

revans2 commented Jan 11, 2017

Uh oh!

ppoulosk Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

revans2 Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

ppoulosk Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

harshach Jan 14, 2017

Choose a reason for hiding this comment

Uh oh!

harshach Jan 14, 2017

Choose a reason for hiding this comment

Uh oh!

revans2 Jan 17, 2017

Choose a reason for hiding this comment

Uh oh!

harshach commented Jan 14, 2017

Uh oh!

ppoulosk commented Jan 17, 2017

Uh oh!

revans2 commented Jan 17, 2017

Uh oh!

ppoulosk commented Jan 17, 2017

Uh oh!

harshach commented Jan 24, 2017

Uh oh!

revans2 commented Jan 24, 2017

Uh oh!

revans2 commented Jan 30, 2017

Uh oh!

hmcl commented Jan 30, 2017

Uh oh!

HeartSaVioR commented Jan 31, 2017

Uh oh!

revans2 commented Jan 31, 2017

Uh oh!

revans2 commented Jan 31, 2017

Uh oh!

HeartSaVioR commented Feb 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants