STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples #2243

srdo · 2017-07-25T17:55:07Z

See https://issues.apache.org/jira/browse/STORM-2658.

The changes to the Trident examples mainly have to do with the command line arguments not being consistently passed to the topologies, e.g. the broker url was passed to producing topologies but not the consuming topology. The extra parameters have been removed, I think the example code is fine with hard coded topic names.

HeartSaVioR

Overall it looks good. Left comments on guiding how users provide the dependencies manually.

HeartSaVioR · 2017-07-30T22:50:40Z

examples/storm-kafka-client-examples/README.markdown

+```
+will submit the topologies set up by KafkaSpoutTopologyMainNamedTopics to Storm.
+
+Note that this example produces a jar containing all dependencies for ease of use. In a production environment you may want to reduce the jar size by extracting some dependencies (e.g. org.apache.kafka:kafka-clients) from the jar. You can do this by setting the dependencies you don't want to include in the jars to `provided` scope, and then manually copying the dependencies to your Storm extlib directory.


Instead of copying dependencies to the extlib, you can achieve the same thing (or more) via using --artifacts to add dependencies for specific topology while submitting. I think this is simpler and topology-wide, so would love to guide both, or only --artifacts. (We already replaced the guide for how to add dependencies from Storm SQL.)

Please refer https://github.com/apache/storm/blob/master/docs/Command-line-client.md#jar for details.

Thanks, I didn't know about this flag. It's much better, will replace references to extlib.

HeartSaVioR · 2017-07-30T22:55:19Z

examples/storm-kafka-client-examples/pom.xml

@@ -42,7 +42,9 @@
            <groupId>org.apache.storm</groupId>
            <artifactId>storm-kafka</artifactId>
            <version>${project.version}</version>
+            <!-- You can reduce jar size by uncommenting this and putting dependencies in $STORM-HOME/extlib instead of including them in the jar


Same above. Maybe we can let users choose how they provide dependencies, change the sentence to ...uncommenting this and providing dependencies manually. or so. My intention is that we don't recommend putting dependencies to extlib directory, unless they know what they're doing (affecting whole topologies' dependencies)

HeartSaVioR · 2017-07-30T22:55:30Z

examples/storm-kafka-client-examples/pom.xml

@@ -73,19 +75,27 @@
            <groupId>org.apache.storm</groupId>
            <artifactId>storm-kafka-client</artifactId>
            <version>${project.version}</version>
+            <!-- You can reduce jar size by uncommenting this and putting dependencies in $STORM-HOME/extlib instead of including them in the jar


HeartSaVioR · 2017-07-30T22:55:45Z

examples/storm-kafka-client-examples/pom.xml

        </dependency>
        <dependency>
            <groupId>org.apache.kafka</groupId>
            <artifactId>${storm.kafka.artifact.id}</artifactId>
            <version>${storm.kafka.client.version}</version>
+            <scope>compile</scope>
+            <!-- You can reduce jar size by uncommenting this and putting dependencies in $STORM-HOME/extlib instead of including them in the jar


HeartSaVioR · 2017-07-30T22:55:56Z

examples/storm-kafka-client-examples/pom.xml

        </dependency>
        <dependency>
            <groupId>org.apache.kafka</groupId>
            <artifactId>kafka-clients</artifactId>
            <version>${storm.kafka.client.version}</version>
+            <scope>compile</scope>
+            <!-- You can reduce jar size by uncommenting this and putting dependencies in $STORM-HOME/extlib instead of including them in the jar


HeartSaVioR · 2017-07-31T13:51:42Z

+1

hmcl

I am in favor of the changes proposed in this JIRA. However, my understanding is that this patch is removing the ability to specify the topic names from the CLI, as well as running the topology in LocalCluster. I think these are valid options that we should keep.

Since we are already refactoring, I am also suggesting a few small name changes.

Once we have all the +1s and are in agreement, let's squash all the commits into one.

hmcl · 2017-07-31T18:06:55Z

examples/storm-kafka-client-examples/README.markdown

+## Usage
+This module contains example topologies demonstrating storm-kafka-client spout and Trident usage.
+
+The module is built by `mvn clean package`. This will generate the `target/storm-kafka-client-examples-VERSION.jar` file. The jar contains all dependencies and can be submitted to Storm via the Storm CLI. For example:


... built running ... the Storm CLI, e.g.:

hmcl · 2017-07-31T18:10:47Z

examples/storm-kafka-client-examples/pom.xml

@@ -42,7 +42,9 @@
            <groupId>org.apache.storm</groupId>
            <artifactId>storm-kafka</artifactId>
            <version>${project.version}</version>
+            <!-- You can reduce jar size by uncommenting this and providing the dependencies manually. See the README for details.
            <scope>${provided.scope}</scope>


Isn't the goal of ${provided.scope} to handle the proper scope according to the profile, e.g. just like it is done with the Intellij profile. I am not quite following why the user has to comment/uncomment the scope configuration.

Yes, I somehow missed the dollar. I'll try reverting this and update the readme to set the scope to compile

hmcl · 2017-07-31T18:12:40Z

examples/storm-kafka-client-examples/pom.xml

@@ -73,19 +75,27 @@
            <groupId>org.apache.storm</groupId>
            <artifactId>storm-kafka-client</artifactId>
            <version>${project.version}</version>
+            <!-- You can reduce jar size by uncommenting this and providing the dependencies manually. See the README for details.
            <scope>${provided.scope}</scope>


Isn't the goal of ${provided.scope} to handle the proper scope according to the profile, e.g. just like it is done with the Intellij profile. I am not quite following why the user has to comment/uncomment the scope configuration.

hmcl · 2017-07-31T18:23:47Z

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

-            Thread.sleep(2000);
-            DrpcResultsPrinter.remoteClient().printResults(60, 1, TimeUnit.SECONDS);
-        }
+    protected void run(String[] args) throws AlreadyAliveException, InvalidTopologyException,


why remove the ability to specify the topic name from the command line ?

The topic name wasn't being passed to the consumer before, only the producers as far as I could tell, so if you used the parameters the example didn't work. Fixing it caused a conflict with the wildcard example, because I'd have to change the newKafkaSpoutConfig signature to take a list of topics. That won't work with the Pattern required by the wildcard example. It seemed easier to just remove the option.

hmcl · 2017-07-31T18:35:09Z

.../src/test/java/org/apache/storm/kafka/spout/builders/SingleTopicKafkaSpoutConfiguration.java

@@ -21,15 +21,10 @@
 import static org.apache.storm.kafka.spout.KafkaSpoutConfig.FirstPollOffsetStrategy.EARLIEST;

 import org.apache.kafka.clients.consumer.ConsumerConfig;


I would call package where this class lives config.builder instead of builders, which is a bit misleading since this is really a configuration class.

I also would call the two getXyz methods in this class createXyz, as they are static factory methods. I know that the name was already like that, but since we are changing it, we should just make it more conventional.

Sure, will rename

hmcl · 2017-07-31T18:45:02Z

...mples/src/main/java/org/apache/storm/kafka/spout/test/KafkaSpoutTopologyMainNamedTopics.java

+        new KafkaSpoutTopologyMainNamedTopics().runMain(args);
+    }
+
+    protected void runMain(String[] args) throws Exception {


Isn't this change removing the ability to run this code in LocalCluster mode? I think it is very useful. For example, I use it all the time to run these simple test examples from IntelliJ.

Yes. I'll restore that bit

I remembered why I removed it. LocalCluster is in the storm-server jar, which isn't included by the example projects. I think including it would cause conflict when the jar is deployed to a real cluster. How about I move the ability to run this from a local cluster to a test class? That should still leave people able to run on a local cluster from an IDE, but doesn't interfere with the generated jar.

hmcl · 2017-07-31T18:56:21Z

examples/storm-kafka-client-examples/pom.xml

        </dependency>
        <dependency>
            <groupId>org.apache.kafka</groupId>
            <artifactId>kafka-clients</artifactId>
            <version>${storm.kafka.client.version}</version>
+            <scope>compile</scope>
+            <!-- You can reduce jar size by uncommenting this and providing the dependencies manually. See the README for details.


Isn't the goal of ${provided.scope} to handle the proper scope according to the profile, e.g. just like it is done with the Intellij profile. I am not quite following why the user has to comment/uncomment the scope configuration.

hmcl · 2017-07-31T18:56:30Z

examples/storm-kafka-client-examples/pom.xml

        </dependency>
        <dependency>
            <groupId>org.apache.kafka</groupId>
            <artifactId>${storm.kafka.artifact.id}</artifactId>
            <version>${storm.kafka.client.version}</version>
+            <scope>compile</scope>
+            <!-- You can reduce jar size by uncommenting this and providing the dependencies manually. See the README for details.


Isn't the goal of ${provided.scope} to handle the proper scope according to the profile, e.g. just like it is done with the Intellij profile. I am not quite following why the user has to comment/uncomment the scope configuration.

srdo · 2017-07-31T21:04:52Z

@hmcl I think I addressed everything. Please look again. Thanks.

hmcl · 2017-07-31T22:16:19Z

+1. Thanks @srdo

…-examples

HeartSaVioR · 2017-08-02T23:09:41Z

@srdo
I have no idea which branches I need to apply this patch, so please go on merging yourself. Thanks for the patch. :)

srdo · 2017-08-02T23:10:44Z

Will do, thanks @HeartSaVioR :)

HeartSaVioR reviewed Jul 30, 2017

View reviewed changes

hmcl reviewed Jul 31, 2017

View reviewed changes

STORM-2658: Extract storm-kafka-client examples to storm-kafka-client…

9b84248

…-examples

srdo force-pushed the STORM-2658 branch from de90397 to 9b84248 Compare August 1, 2017 13:43

srdo changed the title ~~STORM-2658: Extract storm-kafka-client examples to storm-kafka-client…~~ STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples Aug 1, 2017

asfgit merged commit 9b84248 into apache:master Aug 3, 2017

srdo mentioned this pull request Aug 8, 2017

STORM-2689: Simplify storm-kafka-example… #2268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples #2243

STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples #2243

srdo commented Jul 25, 2017 •

edited

Loading

HeartSaVioR left a comment

HeartSaVioR Jul 30, 2017

srdo Jul 31, 2017

HeartSaVioR Jul 30, 2017

HeartSaVioR Jul 30, 2017

HeartSaVioR Jul 30, 2017

HeartSaVioR Jul 30, 2017

HeartSaVioR commented Jul 31, 2017

hmcl left a comment

hmcl Jul 31, 2017

srdo Jul 31, 2017

hmcl Jul 31, 2017

srdo Jul 31, 2017

hmcl Jul 31, 2017

hmcl Jul 31, 2017

srdo Jul 31, 2017

hmcl Jul 31, 2017

hmcl Jul 31, 2017

srdo Jul 31, 2017

hmcl Jul 31, 2017

srdo Jul 31, 2017

srdo Jul 31, 2017

hmcl Jul 31, 2017

hmcl Jul 31, 2017

srdo commented Jul 31, 2017

hmcl commented Jul 31, 2017

HeartSaVioR commented Aug 2, 2017

srdo commented Aug 2, 2017

		@@ -21,15 +21,10 @@
		import static org.apache.storm.kafka.spout.KafkaSpoutConfig.FirstPollOffsetStrategy.EARLIEST;

		import org.apache.kafka.clients.consumer.ConsumerConfig;

STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples #2243

STORM-2658: Extract storm-kafka-client examples to storm-kafka-client-examples #2243

Conversation

srdo commented Jul 25, 2017 • edited Loading

HeartSaVioR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeartSaVioR commented Jul 31, 2017

hmcl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo commented Jul 31, 2017

hmcl commented Jul 31, 2017

HeartSaVioR commented Aug 2, 2017

srdo commented Aug 2, 2017

srdo commented Jul 25, 2017 •

edited

Loading