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-1595; remove global lock from json parser #2214

Closed
wants to merge 1 commit into from

Conversation

resetius
Copy link
Contributor

@resetius resetius commented Dec 6, 2016

No description provided.

@resetius
Copy link
Contributor Author

resetius commented Dec 6, 2016

This PR just removes global lock.
It can help to speed up the controller.
I recommend to use this patch together with #2213

@ijuma
Copy link
Contributor

ijuma commented Dec 6, 2016

Thanks for the PR. Can you please elaborate on why it's safe to remove the global lock?

@resetius
Copy link
Contributor Author

resetius commented Dec 6, 2016

Previously we used a global object JSON, now we create JSON on each parseFull. I think it can be done w/o locks.

@ijuma
Copy link
Contributor

ijuma commented Dec 6, 2016

Have you measured the cost of creating a new parser each time? A change like this requires a few benchmarks to motivate it.

@resetius
Copy link
Contributor Author

resetius commented Dec 6, 2016

Reasonable. I'll try to do some benchmarks.

@resetius
Copy link
Contributor Author

I got a strange memleak inside new JSON. Now I use ThreadLocal and assume that kafka uses constant number of threads. But of course a better solution is to use some modern json parser like json4s.

@asfbot
Copy link

asfbot commented Dec 19, 2016

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

@asfbot
Copy link

asfbot commented Dec 19, 2016

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

@asfbot
Copy link

asfbot commented Dec 19, 2016

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

@resetius resetius closed this Jul 26, 2017
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