Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-5227: Remove unsafe assertion, make jaas config safer #3037

Closed
wants to merge 1 commit into from

Conversation

rajinisivaram
Copy link
Contributor

No description provided.

@rajinisivaram
Copy link
Contributor Author

@ijuma Can you take a look? Thank you...

The assertion failure in https://builds.apache.org/job/kafka-trunk-jdk7/2176 could be because of the small timing window where the controller thread was shutdown and it released the latch, but the main thread did the check for the thread before the thread itself was terminated. So I have removed the assertion in ZooKeeperTestHarness that I added in the other PR. The failure in KAFKA-5227 did not fail the thread assertion, so it wasn't caused by brokers.

I reversed the ordering of setting the System property and resetting Configuration to avoid other threads (which shouldn't be there) loading Configuration again before the System property is set.

@rajinisivaram
Copy link
Contributor Author

There are tests which leave ZooKeeper clients behind. And the client SendThread loads Configuration. Hopefully this PR will fix the issue without having to track down every thread leak.

@asfbot
Copy link

asfbot commented May 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3833/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented May 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/3845/
Test PASSed (JDK 7 and Scala 2.11).

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. We probably still need to track down tests that are leaking ZK instances as such leaks tend to cause test instability as well.

JaasContextTest also needed to be updated in a similar way as other tests and I'll do that before merging to trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants