You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by cutd <gi...@git.apache.org> on 2016/12/01 03:44:31 UTC
[GitHub] storm pull request #1807: fix NullPointException with acked.get(rtp)
GitHub user cutd opened a pull request:
https://github.com/apache/storm/pull/1807
fix NullPointException with acked.get(rtp)
acked.get(rtp) maybe null when antocommit is true.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cutd/storm patch-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1807.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 #1807
----
commit 786083b27dc9750075dbddc335bd64f3f81e7c0d
Author: Tandy <98...@qq.com>
Date: 2016-12-01T03:43:07Z
fix NullPointException with acked.get(rtp)
acked.get(rtp) maybe null when antocommit is true.
----
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1807
@srdo @cutd looking at 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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/1807
@cutd Great. Could you update the PR so it changes `fail` instead of `doSeekRetriableTopicPartitions`? You might also want to create an issue at https://issues.apache.org/jira/browse/STORM so this fix can be tracked :)
---
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 #1807: fix NullPointException with acked.get(rtp)
Posted by cutd <gi...@git.apache.org>.
Github user cutd closed the pull request at:
https://github.com/apache/storm/pull/1807
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/1807
Two things.
@cutd I am +1 for your patch.
@srdo I see your point. If I understand correctly what you mean is that either the Spout guarantees at least once or it doesn't. If it doesn't, we shouldn't even bother to retry at all. The only thing I wonder is if a tuple fails occasionally, if one should try to retry it from memory, hoping for a best effort. What do you think?
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/1807
@hmcl Yes, the case I'm thinking about is where the topology has acking enabled, but the spout is set to auto-commit mode. It would probably be fine to do best-effort retrying when using that setup, but it seems like kind of an odd case to support. The retry code needs to be changed to support that case as well, since doSeekRetriableTopicPartitions doesn't work at all when auto-commit is enabled, because it depends on acked, so I'm not really sure it's worth it.
Regarding the patch and my previous comment, I still think moving this check into fail() and just disabling retries if auto commit is enabled is a better option (i.e. if auto-commit is on, the only thing fail() needs to do is remove the message from emitted and nothing else). It has the same effect as this patch, but we can ensure that it only has effect if auto-commit is enabled. The patch as is could end up hiding bugs, since doSeekRetriableTopicPartitions now quietly skips topicpartitions where acked is null. It's also easier to tell from reading the code why failing tuples would be disabled when auto-commit is on, than it is to tell why there's a null check in doSeekRetriableTopicPartitions.
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1807
@cutd Any updates here? I'd like to see this PR updated with @srdo recommendation.
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by cutd <gi...@git.apache.org>.
Github user cutd commented on the issue:
https://github.com/apache/storm/pull/1807
@srdo I think you are right!
---
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 issue #1807: fix NullPointException with acked.get(rtp)
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/1807
Good catch. I think I'd prefer disabling the retry logic entirely when auto commit is enabled though. It doesn't really make sense to me that you would want to have Storm guarantee at least once processing for tuples from the Kafka spout, but also don't care about losing tuples if the Kafka spout crashes. It would be good IMO if we added the same type of check to fail() as ack() has, so we skip scheduling tuples for retry if auto commit is enabled.
@hmcl do you have any thoughts on this?
---
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.
---