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

revans2 · 2016-12-01T14:34:29Z

STORM-2228: removed ability to request a single topic go to multiple streams

revans2 · 2016-12-01T14:38:50Z

I updated the docs and the examples. This is a non-backwards compatible change from the 1.x release. I have some code (not included here) that can maintain most compatibility if we really want to, but because of STORM-2228 I decided it would be better to just not support that at all. If we want to add that compatibility for 1.x but deprecated I am OK with that, but would like feedback on it before putting up some kind of a pull request.

d2r · 2016-12-06T21:23:24Z

I see some duplicate code and documentation. Is the idea to move from /external/storm-kafka to /external/storm-kafka-client?

srdo · 2016-12-07T17:41:21Z

docs/storm-kafka-client.md

+#Storm Kafka integration using the kafka-client jar

-Apache Storm Spout implementation to consume data from Apache Kafka versions 0.10 onwards (please see [Apache Kafka Version Compatibility] (#compatibility)). 
+##Compatability


Nitpick: Compatibility is misspelled

srdo · 2016-12-07T17:44:11Z

docs/storm-kafka-client.md

+use the default constructor to construct FieldNameBasedTupleToKafkaMapper for backward compatibility
+reasons. Alternatively you could also specify a different key and message field by using the non default constructor.
+In the TridentKafkaState you must specify what is the field name for key and message as there is no default constructor.
+These should be specified while constructing and instance of FieldNameBasedTupleToKafkaMapper.


Nitpick: and -> an

srdo · 2016-12-07T17:50:53Z

docs/storm-kafka-client.md

+The implementation of this interface should return the topic to which the tuple's key/message mapping needs to be published
+You can return a null and the message will be ignored. If you have one static topic name then you can use
+DefaultTopicSelector.java and set the name of the topic in the constructor.
+`FieldNameTopicSelector` and `FieldIndexTopicSelector` use to support decided which topic should to push message from tuple.


Phrasing is a little awkward here. Maybe something like "... can be used to decide which topic a message should go to based on a field in the tuple" instead?

srdo · 2016-12-07T17:51:54Z

docs/storm-kafka-client.md

+DefaultTopicSelector.java and set the name of the topic in the constructor.
+`FieldNameTopicSelector` and `FieldIndexTopicSelector` use to support decided which topic should to push message from tuple.
+User could specify the field name or field index in tuple ,selector will use this value as topic name which to publish message.
+When the topic name not found , `KafkaBolt` will write messages into default topic .


"not found" -> "is not found"

Nit: I think it would be clearer if this referred to Field*TopicSelector instead of KafkaBolt, I misunderstood it on first read.

srdo · 2016-12-07T17:52:10Z

docs/storm-kafka-client.md

+`FieldNameTopicSelector` and `FieldIndexTopicSelector` use to support decided which topic should to push message from tuple.
+User could specify the field name or field index in tuple ,selector will use this value as topic name which to publish message.
+When the topic name not found , `KafkaBolt` will write messages into default topic .
+Please make sure the default topic have created .


have -> has been

srdo · 2016-12-07T19:17:23Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java


    private Collection<TopicPartition> toArrayList(final TopicPartition tp) {
-        return new ArrayList<TopicPartition>(1){{add(tp);}};
+        ArrayList<TopicPartition> ret = new ArrayList<>(1);


Nit: Unless you need to modify the return value later, you can just return Collections.singleton(tp)

srdo · 2016-12-07T19:18:17Z

...afka-client/src/main/java/org/apache/storm/kafka/spout/trident/KafkaTridentSpoutEmitter.java


    private Collection<TopicPartition> toArrayList(final TopicPartition tp) {
-        return new ArrayList<TopicPartition>(1){{add(tp);}};
+        ArrayList<TopicPartition> ret = new ArrayList<>(1);


Nit: Same as in KafkaSpout

srdo · 2016-12-07T19:20:00Z

...afka-client/src/main/java/org/apache/storm/kafka/spout/trident/KafkaTridentSpoutManager.java

+                fields = translator.getFieldsFor(stream);
+            } else {
+                if (!fields.equals(translator.getFieldsFor(stream))) {
+                    throw new IllegalArgumentException("Trident Spouts do nut support multiple output Fields");


Nit: nut -> not. Also maybe rephrase as "must have the same fields for all streams"

srdo · 2016-12-07T19:23:44Z

external/storm-kafka-client/src/test/java/org/apache/storm/kafka/bolt/KafkaBoltTest.java

+                return null;
+            }
+        });
+        KafkaBolt<String, String> bolt = new KafkaBolt<String, String>() {


Nit: Can leave off the contents of <> on the right hand side in a few places here

In this case because I am subclassing KafkaBolt it the compiler(at least in eclipse) actually complains that I am not allowed to do it.

Right, can't do that. Sorry :)

srdo · 2016-12-07T19:25:24Z

...orm-kafka-client/src/test/java/org/apache/storm/kafka/spout/ByTopicRecordTranslatorTest.java

+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFiledCollision() {


filed -> field

srdo · 2016-12-07T19:35:09Z

I like the new configuration design. Backward compatibility is not an issue for us. I'm wondering if enough people have even switched to the new spout yet to make backward compatibility for 1.x a must.

revans2 · 2016-12-07T23:18:55Z

@srdo for me backwards compatibility for 1.x is more a question of violating out versioning than anything else.

srdo · 2016-12-08T17:28:14Z

Sure, that makes sense. Maybe storm-kafka-client should have been marked unstable since it's fairly new, it's still in a phase where the kinks are being worked out, even if the version number says differently.

revans2 · 2016-12-08T22:15:01Z

@srdo I think I addressed all of your review comments.

srdo · 2016-12-08T22:40:27Z

I think my only remaining nit is the spelling error in KafkaBolt. Thanks @revans2. I am +1 on this.

hmcl · 2016-12-08T23:44:16Z

@revans2 I would like to take a look at this before this gets merged in.

revans2 · 2016-12-12T18:38:14Z

@srdo as part of backporting this to 1.x I am going to need to make a change to not use Function directly, because it is only in java 8. So to maintain compatibility between 1.x and 2.x I am going to need to make a few changes in this patch too.

HeartSaVioR · 2017-01-06T09:07:52Z

external/storm-kafka-client/README.md

-
-## Committer Sponsors
- * Sriharsha Chintalapani ([sriharsha@apache.org](mailto:sriharsha@apache.org))
+Please see [here](../../docs/storm-kafka-client.md)


We still need to keep the description because this file will be included to binary dist whereas docs is not.

revans2 · 2017-01-06T22:43:16Z

I think I have addressed all of the review comments so far. I will try to get my 1.x version of the patch up shortly.

srdo · 2017-01-07T08:33:01Z

external/storm-kafka-client/README.md

+#Storm Kafka integration using the kafka-client jar

-Apache Storm Spout implementation to consume data from Apache Kafka versions 0.10 onwards (please see [Apache Kafka Version Compatibility] (#compatibility)). 
+Spouts and Bolts used to interact with Kafka thought the kafka-client library.


Nit: "Thought" should be "through"

revans2 · 2017-01-09T17:17:55Z

All outstanding review comments should be done now. This and the 1.x port at #1868 should be ready for a final pass and hopefully being merged in.

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

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

revans2 · 2017-01-18T16:44:37Z

@srdo @harshach sorry to do this to you, but I just fixed the conflicts with STORM-2236. Sadly the fastest way I could do it was to revert the original code and implement similar functionality, which is the latest commit. Could you please take a look at it? I created a few new Subscription implementations that can do the manual partition management. It only needed a small change to the Spout to support a timeout. I will try to look at adding some documentation and also the impact to the trident spout.

hmcl · 2017-01-18T17:52:44Z

docs/storm-kafka-client.md

+```
+
+#### Wildcard Topics
+Wildcard topics will consume from all topics that are on the configured broakers and match the pattern.  So in the following example


... that exist in the brokers list specified and...

hmcl · 2017-01-18T17:53:36Z

docs/storm-kafka-client.md

+
+#### Wildcard Topics
+Wildcard topics will consume from all topics that are on the configured broakers and match the pattern.  So in the following example
+"topic", "topic_foo" and "topic_bar" will all match the pattern, but "not_my_topic" would not match. 


the pattern "topic.*" ... "won't match"

revans2 · 2017-01-18T19:35:57Z

The test failures are unrelated

hmcl

@revans2 thanks for your patience waiting for the review, and apologies for taking a little longer to review your patch. I am submitting this batch of comments such that you can address them will I go through the reminder of the code. I will finish my comments today.

hmcl · 2017-01-18T19:08:43Z

docs/storm-kafka-client.md

+a spout.
+
+`bootstrapServers` is the same as the Kafka Consumer Property "bootstrap.servers".
+`topics` The topics the spout will consume can either be a `Collection` of specific topic names (1 or more) or a regular expression `Pattern` and any


... expression, which specifies that any topics ...

hmcl · 2017-01-18T19:12:33Z

docs/storm-kafka-client.md

+`topics` The topics the spout will consume can either be a `Collection` of specific topic names (1 or more) or a regular expression `Pattern` and any
+topics that match that regular expression will be consumed.
+
+In the case of the Constructors you may also need to specify a key deserializer and a value deserializer.  This is to help make the java generics happy


This is to guarantee type safety through the use of Java generics

hmcl · 2017-01-18T19:17:06Z

docs/storm-kafka-client.md

+ * `UNCOMMITTED_EARLIEST` (DEFAULT) means that the kafka spout polls records from the last committed offset, if any. If no offset has been committed, it behaves as `EARLIEST`.
+ * `UNCOMMITTED_LATEST` means that the kafka spout polls records from the last committed offset, if any. If no offset has been committed, it behaves as `LATEST`.
+
+`setRecordTranslator` allows you to modify how the spout converts a `ConsumerRecord` into a Tuple and which stream that tuple will go to.  By default the "topic",


Kafka Consumer Record into a Tuple, and which stream that tuple will be published into.

hmcl · 2017-01-18T19:18:43Z

docs/storm-kafka-client.md

+"partition", "offset", "key", and "value" will be emitted to the "default" stream.  If you want to output entries to different streams based on the topic storm
+provides `ByTopicRecordTranslator`.  See below for more examples on how to use these.
+
+`setProp` can be used to set kafka properties that do not have a convenience method.


"Optional" - should we rename this method to setKafkaProp or setKafkaBrokerProp ?

We are configuring the kafka consumer, so I would assume that it was implied.

hmcl · 2017-01-18T19:20:04Z

docs/storm-kafka-client.md

+### Usage Examples
+
+#### Create a Simple Insecure Spout
+The following will consume all events published to "topic" and send them to MyBolt as "topic", "partition", "offset", "key", "value".


... MyBolt using the fields "topic" ...

hmcl · 2017-01-18T19:49:07Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    }
+
+    /**
+     * Intended to be overridden for tests.  Make the producer from props


Create producer with props.

backwards compatibility

No sorry wrong place for that comment.

hmcl · 2017-01-18T19:51:09Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    private TupleToKafkaMapper<K,V> mapper;
+    private KafkaTopicSelector topicSelector;
+    private Properties boltSpecfiedProperties = new Properties();
+    private boolean fireAndForget = false;


Can you please add a comment saying to look at the comments in the set method? I missed those comments at first.

hmcl · 2017-01-18T19:55:00Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    private KafkaTopicSelector topicSelector;
+    private Properties boltSpecfiedProperties = new Properties();
+    private boolean fireAndForget = false;
+    private boolean async = true;


Can you please add a comment saying to look at the comments in the set method? I missed those comments at first.

hmcl · 2017-01-18T20:00:30Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    }
+
+    /**
+     * If set to true the bold will assume that sending a message to kafka will succeed and will ack


hmcl · 2017-01-18T20:02:21Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    /**
+     * If set to true the bold will assume that sending a message to kafka will succeed and will ack
+     * the tuple as soon as it has handed the message off to the producer API
+     * if false (the default) the message will be acked after it we successfully sent to kafka or


it was successfully

srdo · 2017-01-18T19:58:27Z

external/storm-kafka-client/README.md

@@ -1,191 +1,5 @@
-#Storm Kafka Spout with New Kafka Consumer API
+#Storm Apache Kafka integration using the kafka-client jar (This inclused the new Apache Kafka consumer API)


inclused -> includes

srdo · 2017-01-18T20:00:47Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+ * <p/>
+ * It expects the producer configuration and topic in storm config under
+ * <p/>
+ * 'kafka.broker.properties' and 'topic'


Is kafka.broker.properties still being used somewhere? It might be good to mention that new code should prefer withTopicSelector over using 'topic' in storm conf.

srdo · 2017-01-18T20:09:43Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/KafkaBolt.java

+    }
+
+    /**
+     * If set to true the bold will assume that sending a message to kafka will succeed and will ack


bold -> bolt

srdo · 2017-01-18T20:22:21Z

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

+    private final Map<String, RecordTranslator<K,V>> topicToTranslator = new HashMap<>();
+    private final Map<String, Fields> streamToFields = new HashMap<>();
+
+    public ByTopicRecordTranslator(Func<ConsumerRecord<K, V>, List<Object>> func, Fields fields, String stream) {


Nit: This class could do with some javadoc

srdo · 2017-01-18T20:23:30Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java

    private transient Set<KafkaSpoutMessageId> emitted;                 // Tuples that have been emitted but that are "on the wire", i.e. pending being acked or failed
    private transient Iterator<ConsumerRecord<K, V>> waitingToEmit;         // Records that have been polled and are queued to be emitted in the nextTuple() call. One record is emitted per nextTuple()
    private transient long numUncommittedOffsets;                       // Number of offsets that have been polled and emitted but not yet been committed
+	private transient TopologyContext context;


Nit: The rest of the file uses spaces for indentation

srdo · 2017-01-18T20:47:17Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java

    public Deserializer<K> getKeyDeserializer() {
-        return keyDeserializer;
+    	if (keyDesClazz != null) {
+    		try {


srdo · 2017-01-18T20:51:37Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java

-         * If set to true, the consumer manage partition manually, otherwise it will rely on kafka to do partition assignment.
-         * @param manualPartitionAssignment True if using manual partition assignment.
+         * Sets partition refresh period in milliseconds. This is how often the subscription is refreshed
+         * For most subscriptions that go through the KafkaConsumer.subscribe this is ignored.


This seems vague, and requires the user to go look at the subscription source. Maybe either link the method in Subscription that must be overridden to make this apply, or list the subscriptions that don't ignore this setting

srdo · 2017-01-18T21:05:07Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/Subscription.java

+    }
+
+    /**
+     * @return a string representing the subscribed topics.


Can this string be in any format? It seems to only be used in getComponentConfiguration, not really sure where that's being read.

I didn't know either, but the only place I can see it used is in logging so that is what I did.

srdo · 2017-01-18T21:11:42Z

external/storm-kafka/src/jvm/org/apache/storm/kafka/bolt/KafkaBolt.java

 * It works for sending tuples to older Kafka version (0.8.1).
+ * @deprecated Please use the KafkaBolt in storm-kafka-client
 */
+@Deprecated


Maybe it would be fine to just remove it here immediately and deprecate it in 1.1.0?

I would rather do that in a follow on JIRA. Most of the code is the same between the two so getting the rework from here to make there is a lot simpler if I can do a cherry pick

I filed https://issues.apache.org/jira/browse/STORM-2317 to do this and I assigned it to myself.

srdo · 2017-01-18T21:12:21Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpoutConfig.java

+         * The maximum number of records a poll will return.
+         * Will only work with Kafka 0.10.0 and above.
+         */
+        public Builder<K,V> setMaxPoolRecords(int records) {


maxPool -> maxPoll

hmcl

@revans2 This completes my review

hmcl · 2017-01-18T20:08:43Z

.../storm-kafka-client/src/main/java/org/apache/storm/kafka/bolt/mapper/TupleToKafkaMapper.java

+import java.io.Serializable;
+
+/**
+ * as the really verbose name suggests this interface maps a storm tuple to kafka key and message.


I would delete "as the really verbose name suggests this " and put "interface defining a mapping from storm tuple to kafka key and message"

hmcl · 2017-01-18T21:45:20Z

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

+        if (fieldIndex < tuple.size()) {
+            return tuple.getString(fieldIndex);
+        } else {
+            LOG.warn("Field Index " + fieldIndex + " Out of bound . Using default topic " + defaultTopicName);


bounds. Returning default ...

hmcl · 2017-01-18T21:59:01Z

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

+    }
+
+    @Override
+    public String getTopic(Tuple tuple) {


This logic could be incorporated in the FieldIndexTopicSelector. It should be possible to delete this class, rename FieldIndexTopicSelector to FieldTopicSelector, and provide two constructors. One with fieldName and another with fieldIndex. And then use Tuple#fieldIndex(String field) method to extract one from the other.

I agree but both for backwards compatibility and to limit the scope of this JIRA I would rather see it done in a follow on JIRA

I filed https://issues.apache.org/jira/browse/STORM-2318 for this

hmcl · 2017-01-18T22:02:27Z

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

+ * Uses field with a given index to select the topic name from a tuple .
+ */
+public class FieldIndexTopicSelector implements KafkaTopicSelector {
+    private static final long serialVersionUID = -3830575380208166367L;


Why does this class and other classes (e.g. ByTopicRecordTranslator) have serialVersionUID but others (e.g. DefaultTopicSelector, FieldNameTopicSelector) don't?

Because I missed them.

hmcl · 2017-01-18T22:41:02Z

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

+            Fields fromTrans = translator.getFieldsFor(stream);
+            Fields cached = streamToFields.get(stream);
+            if (cached != null && !fromTrans.equals(cached)) {
+                throw new IllegalArgumentException("Stream " + stream + " currently has Fields of " + cached + " which is not the same as those being added in " + fromTrans);


IllegalStateException ?

In this case it is the argument being passed in that is bad and we are rejecting it. They could come back and switch it to a new stream which would be fine.

hmcl · 2017-01-19T02:13:09Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java

+    private TridentTupleToKafkaMapper mapper;
+    private KafkaTopicSelector topicSelector;
+
+    public TridentKafkaState withTridentTupleToKafkaMapper(TridentTupleToKafkaMapper mapper) {


Wouldn't it be preferable the class to be immutable ?

This is code that was "moved" like with the KafkaBolt for storm-kafka to storm-kafka-client. If we really want to make it immutable we can, but I think that is beyond the scope of this JIRA

hmcl · 2017-01-19T02:14:24Z

external/storm-kafka-client/src/main/java/org/apache/storm/kafka/trident/TridentKafkaState.java

+    }
+
+    public void prepare(Properties options) {
+        if (mapper == null) throw new NullPointerException("mapper can not be null");


Objects.requireNonNull("mapper cannot be null");
..

Objects does not exist in java 6 and I would prefer to keep the code compatible as mush as possible to avoid extra rework when pulling these changes back. If you insist I will do it.

I thought 1.x was targeting at least Java 7?

@srdo you are right.

<artifactId>maven-compiler-plugin</artifactId> <configuration> <source>1.7</source> <target>1.7</target> </configuration>

I'll update it.

hmcl · 2017-01-19T02:19:19Z

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

+import java.io.Serializable;
+
+public interface KafkaTopicSelector extends Serializable {
+    String getTopic(Tuple tuple);


since in some scenarios it's OK that this value returns null, wouldn't it be much clearer to return Optional<String> instead?

File another JIRA and we can look into it. I think it is beyond the scope of this one.

IMHO we need to try avoiding JDK 8 feature for public API unless we decided to release minor version only for 2.0.0. But anyway I also think this is beyond of the scope.

hmcl · 2017-01-19T02:23:35Z

...nt/src/main/java/org/apache/storm/kafka/trident/mapper/FieldNameBasedTupleToKafkaMapper.java

+
+    @Override
+    public K getKeyFromTuple(TridentTuple tuple) {
+        return (K) tuple.getValueByField(keyFieldName);


ClassCastException?

It is possible but that is the point of the generics. To try and reduce the likelihood of it happening.

hmcl · 2017-01-19T02:23:40Z

...nt/src/main/java/org/apache/storm/kafka/trident/mapper/FieldNameBasedTupleToKafkaMapper.java

+    }
+
+    @Override
+    public V getMessageFromTuple(TridentTuple tuple) {


ClassCastException?

It is possible but that is the point of the generics. To try and reduce the likelihood of it happening.

revans2 · 2017-01-19T18:34:27Z

@hmcl I think I addressed all of your review comments. I also re-based to deal with the merge conflict. If everything looks good I will squash some of the commits. There are a lot.

revans2 · 2017-01-19T19:19:28Z

The travis test failures are unrelated.

srdo · 2017-01-24T18:25:24Z

There are still a few files with tabs mixed in to spaces, so autoformatting all the files might be good. I also still don't really understand why the KafkaSpoutConfig builder needs to support setting a deserializer with a different generic type to the one the builder was instantiated with.

+1 though.

ptgoetz · 2017-01-25T21:53:49Z

+1

hmcl · 2017-01-26T16:52:29Z

+1

My only suggestion at this point is that we file an umbrella JIRA covering some of the follow up comments that haven't been addressed, such as minor refactoring, improved (java) docs, immutability, etc. Basically to just capture the most meaningful comments that makes sense handle later.

Thanks @revans2 for all your patience addressing the many review comments

…into STORM-2236" This reverts commit e2f87c3, reversing changes made to 00fed51.

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-30T20:46:15Z

Everyone,

Sorry this took so long. I kept getting pulled off on critical issues. I have rebased/squashed everything (except the code revert) and also fixed the issue with tabs vs spaces. Since it was just white space issues I don't think I need more +1s but if someone wants to take a look again please feel free to. I will do the same for the 1.x version, and then file the umbrella jira that @hmcl requested.

hmcl · 2017-01-30T21:03:22Z

Thanks @revans2. Can you please just amend the commit log messages as I believe they do not really reflect the changes in this patch. One of the messages has STORM-1997 title, and the other has a cryptic merge log message.

Also, can you please do the same for 1.x-branch patch

revans2 · 2017-01-30T21:10:57Z

@hmcl github like to truncate long messages.

$ git log STORM-2225
commit c9f9348e3de0917020a8c903f4c9d026b7dc6204
Author: Robert (Bobby) Evans <evans@yahoo-inc.com>
Date:   Tue Nov 29 21:39:26 2016 -0600

    STORM-1997: copy state/bolt from storm-kafka to storm-kafka-client
    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

commit 74a77aa6494957b941df55d66ff64b6561eee4a8
Author: Robert (Bobby) Evans <evans@yahoo-inc.com>
Date:   Wed Jan 18 10:34:30 2017 -0600

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

    This reverts commit e2f87c304297ce95f01708e0934c44ee385fa0bb, reversing
    changes made to 00fed518d14b485e2a9e9cdd91ade3cc9d68dc8d.

If you really want me to squash the code change with the revert and only reference a single JIRA in the commit messages I can. But I thought it would be better if I am addressing 4 separate (but very closely related) JIRA I should keep them all together.

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

@revans2 this is perfect. These 4 commits make it much easier to track down. I confused the way GitHub truncates the messages - my bad.

+1

HeartSaVioR · 2017-01-31T00:52:03Z

Thanks for the great work and patience. +1

revans2 changed the title ~~STORM-1997: copy state/bolt from storm-kafka to storm-kafka-client~~ STORM-2225: change spout config to be simpler. Dec 1, 2016

srdo reviewed Dec 7, 2016

View reviewed changes

revans2 force-pushed the STORM-2225 branch from 32da09c to 1d1942e Compare December 8, 2016 22:10

HeartSaVioR reviewed Jan 6, 2017

View reviewed changes

revans2 force-pushed the STORM-2225 branch from 931e10c to 17da056 Compare January 6, 2017 21:07

srdo reviewed Jan 7, 2017

View reviewed changes

revans2 mentioned this pull request Jan 9, 2017

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

Merged

revans2 force-pushed the STORM-2225 branch from bbed826 to 9d608fe Compare January 18, 2017 16:39

hmcl reviewed Jan 18, 2017

View reviewed changes

srdo reviewed Jan 18, 2017

View reviewed changes

hmcl reviewed Jan 19, 2017

View reviewed changes

revans2 force-pushed the STORM-2225 branch from 06c7840 to c941216 Compare January 19, 2017 18:32

srdo mentioned this pull request Jan 24, 2017

STORM-2250: Kafka Spout Refactoring to Increase Modularity and Testability #1832

Merged

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

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

74a77aa

…into STORM-2236" This reverts commit e2f87c3, reversing changes made to 00fed51.

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

c9f9348

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 branch from 32e7bcd to c9f9348 Compare January 30, 2017 20:42

asfgit merged commit c9f9348 into apache:master Feb 1, 2017

		@@ -1,191 +1,5 @@
		#Storm Kafka Spout with New Kafka Consumer API
		#Storm Apache Kafka integration using the kafka-client jar (This inclused the new Apache Kafka consumer API)

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

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

Uh oh!

Conversation

revans2 commented Dec 1, 2016

Uh oh!

revans2 commented Dec 1, 2016

Uh oh!

d2r commented Dec 6, 2016

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!

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 Dec 7, 2016

Uh oh!

revans2 commented Dec 7, 2016

Uh oh!

srdo commented Dec 8, 2016

Uh oh!

revans2 commented Dec 8, 2016

Uh oh!

srdo commented Dec 8, 2016

Uh oh!

hmcl commented Dec 8, 2016

Uh oh!

revans2 commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 commented Jan 9, 2017

Uh oh!

revans2 commented Jan 11, 2017

Uh oh!

revans2 commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmcl Jan 18, 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!

revans2 commented Jan 18, 2017

Uh oh!

hmcl left a comment

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

hmcl Jan 18, 2017 •

edited

Loading