You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Zhiqiang-He <gi...@git.apache.org> on 2015/11/17 10:22:21 UTC
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
GitHub user Zhiqiang-He opened a pull request:
https://github.com/apache/storm/pull/885
[STORM-1210] Set Output Stream id in KafkaSpout
topicAsStreamId can only set output stream id to topic name. In some case ,we need to set output stream id to other name.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Zhiqiang-He/storm-0914-edit master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/885.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 #885
----
commit c1a89d9597dfcbfeca260d4264a4b2e374c419be
Author: Zhiqiang He <ab...@qq.com>
Date: 2015-10-21T02:09:12Z
Merge pull request #1 from apache/master
merge 1021
commit 5261160f84bcb0ef73acf4a632d421f7e2ee901d
Author: Zhiqiang He <ab...@qq.com>
Date: 2015-11-17T02:57:09Z
Merge pull request #2 from apache/master
merge 1117
commit 87a4a8b4382de5617d463b23188f18e4f7e7f03c
Author: Zhiqiang-He <ab...@qq.com>
Date: 2015-11-17T09:14:54Z
set output stream id in kafkaSpout
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-168816433
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by ppoulosk <gi...@git.apache.org>.
Github user ppoulosk commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-164551153
+1, NB
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-161507454
+1 Nice work. It is much clearer now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by Zhiqiang-He <gi...@git.apache.org>.
Github user Zhiqiang-He commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-164617141
@rfarivar Because guava is already import in storm-kafka.
And com.google.common.collect.ImmutableMap is already used in PartitionManager.java.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-157341386
Looks good to me.
But the condition code is a little reduplicate and verbose , I prefer to check only one variable to set streamId with reseting outputStreamId in SpoutConfig constructor.
```java
this.outputStreamId = topicAsStreamId ? topic : this.outputStreamId;
```
Not a big deal, just something trivial .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/885
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by rfarivar <gi...@git.apache.org>.
Github user rfarivar commented on a diff in the pull request:
https://github.com/apache/storm/pull/885#discussion_r47586077
--- Diff: external/storm-kafka/src/jvm/storm/kafka/PartitionManager.java ---
@@ -149,9 +150,9 @@ public EmitState next(SpoutOutputCollector collector) {
}
if ((tups != null) && tups.iterator().hasNext()) {
- if(_spoutConfig.topicAsStreamId) {
+ if (!Strings.isNullOrEmpty(_spoutConfig.outputStreamId)) {
--- End diff --
Isn't it an overkill to import a whole package (com.google.common.base.Strings) to only test for null or empty? Can't we just use:
if (_spoutConfig.outputStreamId!=null && _spoutConfig.outputStreamId!="")
or something to that effect instead of importing a new library?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-157342872
Looks good to me.
But the condition code (`topicAsStreamId` and `outputStreamId`) is a little reduplicate as the logic is almost the same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-168817797
Still +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by Zhiqiang-He <gi...@git.apache.org>.
Github user Zhiqiang-He commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-157915066
@jerrypeng
outputStreamId is used in declareStream as emit method.
It is very useful when bolt after kafkaspout recive two or more stream messages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-159295188
+1 the code looks good to me. I agree with @wuchong that the logic is very similar between the two and there is the potential to combine some code, them but it is small enough I don't think it is that important.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/storm/pull/885#discussion_r46510092
--- Diff: external/storm-kafka/src/jvm/storm/kafka/SpoutConfig.java ---
@@ -27,8 +27,7 @@
public String zkRoot = null;
public String id = null;
- // if set to true, spout will set Kafka topic as the emitted Stream ID
- public boolean topicAsStreamId = false;
+ public String outputStreamId;
--- End diff --
Minor, but we can note here that if outputStreamId not set, spout will set Kafka topic as the emitted Stream ID
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-157831515
just a quick question. where is outputStreamId set?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by Zhiqiang-He <gi...@git.apache.org>.
Github user Zhiqiang-He commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-157344649
@wuchong
Yes, you are right. but for compatibility reasons, I add this argument based on topicAsStreamId .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1210] Set Output Stream id in KafkaSpou...
Posted by Zhiqiang-He <gi...@git.apache.org>.
Github user Zhiqiang-He commented on the pull request:
https://github.com/apache/storm/pull/885#issuecomment-161506356
@revans2 I already removed topicAsStreamId, please review it. thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---