You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kevinconaway <gi...@git.apache.org> on 2017/06/29 20:58:09 UTC
[GitHub] storm pull request #2182: STORM-2608 Remove any pending offsets that are no ...
GitHub user kevinconaway opened a pull request:
https://github.com/apache/storm/pull/2182
STORM-2608 Remove any pending offsets that are no longer valid
Fixes for https://issues.apache.org/jira/browse/STORM-2608
@abhishekagarwal87 or @revans2 please let me know your thoughts here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kevinconaway/storm storm-2608
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2182.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 #2182
----
commit cf60c15b7ee90c1b5c27f39c9cb4a21c7583aa2c
Author: Kevin Conaway <ke...@walmart.com>
Date: 2017-06-29T20:53:46Z
STORM-2608 Remove any pending offsets that are no longer valid
----
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
Thanks for your patience on this. I did some debugging on Travis on my fork and found 2 issues
- The behavior of binding to _localhost_ seems somewhat inconsistent for Zookeeper and Kafka. I noticed a bunch of ZK connection issues trying to connect to the hostname of the travis machine that wouldn't resolve. Using _127.0.0.1_ fixes this
- More importantly, there were timing issues related to creating the topic and the partition leaders being assigned. `DynamicBrokersReader#getNumPartitions` will fail if the topic doesn't yet have leaders assigned to its partitions and this was the root of the NPE.
I fixed the second issue by waiting until the topic is fully ready in kafka (exists in zookeeper and all partitions have leaders) before attempting to use it. I also adjusted the logic around emitting messages from the partition manager to allow for timing delays on the broker end as this was another cause of spurious failures.
I've also adjusted the _integration-test_ parent pom version to match the current one in the branch. It looks like this was missed when the version was bumped to _1.2.0-SNAPSHOT_
---
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 #2182: STORM-2608 Remove any pending offsets that are no ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2182#discussion_r125297056
--- Diff: external/storm-kafka/src/test/org/apache/storm/kafka/PartitionManagerTest.java ---
@@ -0,0 +1,202 @@
+package org.apache.storm.kafka;
--- End diff --
Apache license header is missing.
---
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 #2182: STORM-2608 Remove any pending offsets that are no ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway closed the pull request at:
https://github.com/apache/storm/pull/2182
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
It succeeds for me locally. The NPE that you mentioned is actually due to another error in the `@Before` method
```
test2608(org.apache.storm.kafka.PartitionManagerTest) Time elapsed: 1.158 sec <<< ERROR!
java.lang.RuntimeException: java.lang.RuntimeException: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /brokers/topics/testTopic/partitions
at org.apache.zookeeper.KeeperException.create(KeeperException.java:111)
at org.apache.zookeeper.KeeperException.create(KeeperException.java:51)
at org.apache.zookeeper.ZooKeeper.getChildren(ZooKeeper.java:1590)
at org.apache.curator.framework.imps.GetChildrenBuilderImpl$3.call(GetChildrenBuilderImpl.java:230)
at org.apache.curator.framework.imps.GetChildrenBuilderImpl$3.call(GetChildrenBuilderImpl.java:219)
at org.apache.curator.RetryLoop.callWithRetry(RetryLoop.java:109)
at org.apache.curator.framework.imps.GetChildrenBuilderImpl.pathInForeground(GetChildrenBuilderImpl.java:216)
at org.apache.curator.framework.imps.GetChildrenBuilderImpl.forPath(GetChildrenBuilderImpl.java:207)
at org.apache.curator.framework.imps.GetChildrenBuilderImpl.forPath(GetChildrenBuilderImpl.java:40)
at org.apache.storm.kafka.DynamicBrokersReader.getNumPartitions(DynamicBrokersReader.java:110)
at org.apache.storm.kafka.DynamicBrokersReader.getBrokerInfo(DynamicBrokersReader.java:83)
at org.apache.storm.kafka.trident.ZkBrokerReader.<init>(ZkBrokerReader.java:44)
at org.apache.storm.kafka.PartitionManagerTest.setup(PartitionManagerTest.java:81)
test2608(org.apache.storm.kafka.PartitionManagerTest) Time elapsed: 1.159 sec <<< ERROR!
java.lang.NullPointerException: null
at org.apache.storm.kafka.PartitionManagerTest.shutdown(PartitionManagerTest.java:102)
```
The `ZkBrokerReader` isn't seeing the topic information in Zookeeper which makes me think its a race condition between when the topic is created and then `ZkBrokerReader` tries to read the topic / partition info
---
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 #2182: STORM-2608 Remove any pending offsets that are no ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on a diff in the pull request:
https://github.com/apache/storm/pull/2182#discussion_r125704833
--- Diff: external/storm-kafka/src/test/org/apache/storm/kafka/PartitionManagerTest.java ---
@@ -0,0 +1,202 @@
+package org.apache.storm.kafka;
--- End diff --
Added
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
Your patch seems not remedy the NPE issue. Same exception raised for newer build.
https://travis-ci.org/apache/storm/jobs/251340422
https://travis-ci.org/apache/storm/jobs/251340423
```
classname: org.apache.storm.kafka.PartitionManagerTest / testname: test2608
java.lang.NullPointerException: null
at org.apache.storm.kafka.PartitionManagerTest.shutdown(PartitionManagerTest.java:103)
```
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway Sure. I'll review the test code and let you know.
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway
Seems like it is failing intermittently, but high probability. I also saw test failing on my local, but also saw test succeed on my local.
Could you run tests several times and see how many time it fails?
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
I borked the history trying to squash the commits. I've created a separate PR:
https://github.com/apache/storm/pull/2190
Closing this out
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway Your last change seemed to make compilation failure. Could you ensure that it succeeds to build with both JDK7 and JDK8?
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
I've added a test case for this. Its a bit complicated due to the specific nature of how the bug occurs so I'm open to suggestions on how to improve 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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
> I'd like to see the PR against master branch too. Please note that checkstyle is applied to the master branch.
Sure, I'll wait until this is finalized and then submit a PR with the changes against master. Does that work?
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway
Great. Thanks for the patience on fixing all the issues. +1
Could you squash the commits into one? And please craft another PR against master branch.
Thanks in advance!
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway
Builds in Travis CI seem to fail consistently at this point:
```
classname: org.apache.storm.kafka.PartitionManagerTest / testname: test2608
java.lang.NullPointerException: null
at org.apache.storm.kafka.PartitionManagerTest.shutdown(PartitionManagerTest.java:102)
```
I think this is really odd but happens anyway. Seems like running test cluster inside of test seems not working on Travis CI build.
https://travis-ci.org/apache/storm/jobs/250442626
If you're willing to resolve the build failure, please enable Travis CI in your fork, and fix it. I'm even OK to remove the test code and just having one-liner 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 issue #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by kevinconaway <gi...@git.apache.org>.
Github user kevinconaway commented on the issue:
https://github.com/apache/storm/pull/2182
The constructor for `ZkCoordinator` changed in the upstream branch. Will fix.
---
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 #2182: STORM-2608 Remove any pending offsets that are no longer ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2182
@kevinconaway
I don't mind the change is not covered by test since the change is really small, but adding a new test is better for sure.
The change looks good, but I didn't have time to check the test code. I'll revisit this soon.
Btw, I'd like to see the PR against master branch too. Please note that checkstyle is applied to the master branch.
---
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.
---