You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by prokopowicz <gi...@git.apache.org> on 2015/09/23 17:14:04 UTC
[GitHub] storm pull request: advance kafka offset when deserializer yields ...
GitHub user prokopowicz opened a pull request:
https://github.com/apache/storm/pull/758
advance kafka offset when deserializer yields no object
also removes some tab-indenting in the surrounding code.
When a custom scheme returns an empty list of object-lists to the partition manager, it should be handled the same as null. Otherwise, the partition reader will be stuck at that offset.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/prokopowicz/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/758.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 #758
----
commit 5d499854bf4b236b2d5fc7078d7fc5343d89dbeb
Author: Pete Prokopowicz <pp...@groupon.com>
Date: 2015-09-23T15:09:23Z
advance kafka offset when deserializer yields no object
----
---
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: advance kafka offset when deserializer yields ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/758#issuecomment-145993674
+1, too.
@prokopowicz
Since this is a bug, I'd like to record this to JIRA issue before merging. Could you file a JIRA, please?
---
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: advance kafka offset when deserializer yields ...
Posted by prokopowicz <gi...@git.apache.org>.
Github user prokopowicz commented on a diff in the pull request:
https://github.com/apache/storm/pull/758#discussion_r40970682
--- Diff: external/storm-kafka/src/jvm/storm/kafka/PartitionManager.java ---
@@ -136,15 +136,15 @@ public EmitState next(SpoutOutputCollector collector) {
return EmitState.NO_EMITTED;
}
Iterable<List<Object>> tups = KafkaUtils.generateTuples(_spoutConfig, toEmit.msg);
- if (tups != null) {
- if(_spoutConfig.topicAsStreamId) {
- for (List<Object> tup : tups) {
- collector.emit(_spoutConfig.topic, tup, new KafkaMessageId(_partition, toEmit.offset));
- }
- } else {
- for (List<Object> tup : tups) {
- collector.emit(tup, new KafkaMessageId(_partition, toEmit.offset));
- }
+ if ((tups != null) && tups.iterator.hasNext()) {
--- End diff --
sorry :( I am pushing another commit 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: advance kafka offset when deserializer yields ...
Posted by Crim <gi...@git.apache.org>.
Github user Crim commented on the pull request:
https://github.com/apache/storm/pull/758#issuecomment-146187081
+1 running into this issue as well
---
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: advance kafka offset when deserializer yields ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/758#issuecomment-146770261
Great, I'll merge it.
---
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: advance kafka offset when deserializer yields ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/758#discussion_r40843072
--- Diff: external/storm-kafka/src/jvm/storm/kafka/PartitionManager.java ---
@@ -136,15 +136,15 @@ public EmitState next(SpoutOutputCollector collector) {
return EmitState.NO_EMITTED;
}
Iterable<List<Object>> tups = KafkaUtils.generateTuples(_spoutConfig, toEmit.msg);
- if (tups != null) {
- if(_spoutConfig.topicAsStreamId) {
- for (List<Object> tup : tups) {
- collector.emit(_spoutConfig.topic, tup, new KafkaMessageId(_partition, toEmit.offset));
- }
- } else {
- for (List<Object> tup : tups) {
- collector.emit(tup, new KafkaMessageId(_partition, toEmit.offset));
- }
+ if ((tups != null) && tups.iterator.hasNext()) {
--- End diff --
This doesn't even compile.
>[ERROR] /home/travis/build/apache/storm/external/storm-kafka/src/jvm/storm/kafka/PartitionManager.java:[139,39] cannot find symbol
>
> symbol: variable iterator
>
> location: variable tups of type java.lang.Iterable<java.util.List<java.lang.Object>>
I think it needs to be `tups.iterator().hasNext()`
---
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: advance kafka offset when deserializer yields ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/758#issuecomment-145252882
@prokopowicz I am +1 for merging this in. Is there a JIRA associated with this change?
---
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: advance kafka offset when deserializer yields ...
Posted by prokopowicz <gi...@git.apache.org>.
Github user prokopowicz commented on the pull request:
https://github.com/apache/storm/pull/758#issuecomment-146227524
I issued this JIRA regarding the bug:
https://issues.apache.org/jira/browse/STORM-1094
On Tue, Oct 6, 2015 at 3:40 PM, Jungtaek Lim <no...@github.com>
wrote:
> +1, too.
> @prokopowicz <https://github.com/prokopowicz>
> Since this is a bug, I'd like to record this to JIRA issue before merging.
> Could you file a JIRA, please?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/storm/pull/758#issuecomment-145993674>.
>
---
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: advance kafka offset when deserializer yields ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/758
---
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.
---