Skip to content

KAFKA-1387: Kafka getting stuck creating ephemeral node it has already created when two zookeeper sessions are established in a very short period of time#178

Closed
fpj wants to merge 35 commits into
apache:trunkfrom
fpj:1387
Closed

Conversation

@fpj

@fpj fpj commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

This is a patch to get around the problem discussed in the KAFKA-1387 jira. The tests are not passing in my box when I run them all, but they do pass when I run them individually, which indicates that there is something leaking from a test to the next. I still need to work this out and also work on further testing this. I wanted to open this PR now so that it can start getting reviewed.

fpj
fpj reviewed Aug 29, 2015
View reviewed changes
Comment thread core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala Outdated

@fpj fpj Aug 29, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this commented code.

@asfbot

asfbot commented Aug 29, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #267 FAILURE
Looks like there's a problem with this pull request

@asfbot

asfbot commented Sep 2, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #307 FAILURE
Looks like there's a problem with this pull request

@asfbot

asfbot commented Sep 2, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #308 FAILURE
Looks like there's a problem with this pull request

@asfbot

asfbot commented Sep 2, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #309 FAILURE
Looks like there's a problem with this pull request

@guozhangwang

guozhangwang commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

@fpj Is this ready for review? Could you rebase?

@asfbot

asfbot commented Sep 3, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #343 FAILURE
Looks like there's a problem with this pull request

@fpj

fpj commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

@guozhangwang you can start reviewing if you have time. I'm primarily interested in your opinion about the approach of using a zk handle directly to solve this. I think it is ok, but creates some redudancy in ZkUtils.

Things that I'm not entirely happy with about the patch:
1- Upon a connectionloss event, it'd be better to wait until a connected event before retrying. The way it is isn't wrong, but it will be looping until it reconnects.
2- The current tests cover a lot of the functionality, so I think it is not worse compared to the existing trunk code, but I want to spend a bit more time thinking about test coverage.
3- I'm thinking that we should remove ZkUtils.createEphemeralPathExpectConflictHandleZKBug.

junrao
junrao reviewed Sep 23, 2015
View reviewed changes
Comment thread core/src/main/scala/kafka/utils/ZkUtils.scala Outdated

@junrao junrao Sep 23, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem that we need expectedBroker any more.

@fpj fpj Sep 23, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@asfbot

asfbot commented Sep 24, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #512 FAILURE
Looks like there's a problem with this pull request

@fpj

fpj commented Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

Test failures don't seem to be related to this patch:

kafka.consumer.MetricsTest > testMetricsLeak FAILED
kafka.api.ProducerBounceTest > testBrokerFailure FAILED
kafka.producer.ProducerTest > testSendWithDeadBroker FAILED

junrao
junrao reviewed Sep 24, 2015
View reviewed changes
Comment thread core/src/test/scala/unit/kafka/zk/ZKEphemeralTest.scala Outdated

@junrao junrao Sep 24, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after first comma

@asfbot

asfbot commented Sep 24, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #526 FAILURE
Looks like there's a problem with this pull request

@asfbot

asfbot commented Sep 24, 2015

Copy link
Copy Markdown

kafka-trunk-git-pr #528 FAILURE
Looks like there's a problem with this pull request

@junrao

junrao commented Sep 24, 2015

Copy link
Copy Markdown
Contributor

Thanks for the patch. LGTM.

@asfgit asfgit closed this in 1daf6ac Sep 24, 2015
jsancio pushed a commit to jsancio/kafka that referenced this pull request Aug 6, 2019
@apovzner
CPKAFKA-2468: Make ccloud metadata watcher more reliable (apache#178)
d6a3dba 
* CPKAFKA-2468: Make ccloud metadata watcher more reliable
efeg pushed a commit to efeg/kafka that referenced this pull request Jan 29, 2020
@becketqin @efeg
Bump up Kafka dependency to 0.11.0.2 and gradle version to 4.6 (apach…
1b8eacb 
…e#178)
@chia7712 chia7712 mentioned this pull request Dec 23, 2024
Merged
3 tasks
davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025
@jeqo
fix(inkless:cp): close on replicamanager close (apache#178)
08ea36e
fvaleri pushed a commit to fvaleri/kafka that referenced this pull request May 22, 2026
@showuon
Merge pull request apache#178 from fvaleri/cl3-21-support
0517c31 
Fix source cluster compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fpj @asfbot @guozhangwang @junrao @ijuma