STORM-2608 Remove any pending offsets that are no longer valid #2182

kevinconaway · 2017-06-29T20:58:05Z

Fixes for https://issues.apache.org/jira/browse/STORM-2608

@abhishekagarwal87 or @revans2 please let me know your thoughts here.

kevinconaway · 2017-06-30T16:40:23Z

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.

HeartSaVioR · 2017-07-03T14:05:02Z

@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.

HeartSaVioR · 2017-07-03T14:05:39Z

external/storm-kafka/src/test/org/apache/storm/kafka/PartitionManagerTest.java

@@ -0,0 +1,202 @@
+package org.apache.storm.kafka;


Apache license header is missing.

kevinconaway · 2017-07-05T17:18:17Z

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?

HeartSaVioR · 2017-07-06T00:06:44Z

@kevinconaway Sure. I'll review the test code and let you know.

HeartSaVioR · 2017-07-07T00:09:01Z

@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.

HeartSaVioR · 2017-07-07T03:05:09Z

@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?

kevinconaway · 2017-07-07T21:40:35Z

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

HeartSaVioR · 2017-07-07T22:25:24Z

@kevinconaway Your last change seemed to make compilation failure. Could you ensure that it succeeds to build with both JDK7 and JDK8?

kevinconaway · 2017-07-07T22:32:05Z

The constructor for ZkCoordinator changed in the upstream branch. Will fix.

HeartSaVioR · 2017-07-07T23:31:16Z

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)

kevinconaway · 2017-07-08T22:51:51Z

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

HeartSaVioR · 2017-07-09T01:20:31Z

@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!

…you set it

@arunmahadevan

* Implement HBase state backend * picked Redis state backend implementation * also implemented state iterator * Added 'how to set up' to State-checkpointing doc * address @arunmahadevan's review comment

STORM-2608 Add test case STORM-2608 Add missing license header STORM-2608 Ensure that the topic is created and available in ZK before returning from #createTopic STORM-2608 Update to latest from 1.x branch STORM-2608 Fix IT pom version STORM-2608 Ensure that partitions have leaders before returning from createTopic STORM-2608 Remove any pending offsets that are no longer valid

kevinconaway · 2017-07-09T01:48:55Z

I borked the history trying to squash the commits. I've created a separate PR:

#2190

Closing this out

HeartSaVioR reviewed Jul 3, 2017

View reviewed changes

HeartSaVioR and others added 12 commits July 8, 2017 21:39

STORM-2601: CHANGELOG

897b315

Update target version of 1.x-branch to 1.2.0-SNAPSHOT

789110e

Reflect version 1.2.0 and 1.1.1 to CHANGELOG

4867519

[STORM-2602] storm.zookeeper.topology.auth.payload doesn't work even …

0582f50

…you set it

STORM-2602: CHANGELOG

ce94777

STORM-2506: Print mapping between Task ID and Kafka Partitions

0e08283

STORM-2506: CHANGELOG

25f969c

STORM-2478: Fix BlobStoreTest.testDeleteAfterFailedCreate on Windows

7f6080f

STORM-2478: CHANGELOG

b6eb999

STORM-2383 [storm-hbase] Support HBase as state backend

a5fb685

* Implement HBase state backend * picked Redis state backend implementation * also implemented state iterator * Added 'how to set up' to State-checkpointing doc * address @arunmahadevan's review comment

STORM-2383: CHANGELOG

774d6bb

kevinconaway force-pushed the storm-2608 branch from 0579015 to fe4fce3 Compare July 9, 2017 01:40

kevinconaway closed this Jul 9, 2017

STORM-2608 Remove any pending offsets that are no longer valid #2182

STORM-2608 Remove any pending offsets that are no longer valid #2182

Uh oh!

Conversation

kevinconaway commented Jun 29, 2017

Uh oh!

kevinconaway commented Jun 30, 2017

Uh oh!

HeartSaVioR commented Jul 3, 2017

Uh oh!

HeartSaVioR Jul 3, 2017

Choose a reason for hiding this comment

Uh oh!

kevinconaway Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

kevinconaway commented Jul 5, 2017

Uh oh!

HeartSaVioR commented Jul 6, 2017

Uh oh!

HeartSaVioR commented Jul 7, 2017

Uh oh!

HeartSaVioR commented Jul 7, 2017

Uh oh!

kevinconaway commented Jul 7, 2017

Uh oh!

HeartSaVioR commented Jul 7, 2017

Uh oh!

kevinconaway commented Jul 7, 2017

Uh oh!

HeartSaVioR commented Jul 7, 2017

Uh oh!

kevinconaway commented Jul 8, 2017

Uh oh!

HeartSaVioR commented Jul 9, 2017

Uh oh!

kevinconaway commented Jul 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants