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

Remove temporary usage of a Thrift fork #2781

Closed
jpkrohling opened this issue Feb 3, 2021 · 11 comments · Fixed by #2861
Closed

Remove temporary usage of a Thrift fork #2781

jpkrohling opened this issue Feb 3, 2021 · 11 comments · Fixed by #2861
Assignees
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@jpkrohling
Copy link
Contributor

The PR #2780 introduced the usage of a personal fork for Apache Thrift Go libraries to address a memory problem with the agent. Once a new version of Thrift is released that includes the fix, we need to revert that and use the official library again.

@jpkrohling
Copy link
Contributor Author

Looks like Thrift 0.14 is now released: https://github.com/apache/thrift/releases/tag/v0.14.0

Any takers?

@jpkrohling jpkrohling added enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Feb 16, 2021
@jpkrohling jpkrohling mentioned this issue Feb 16, 2021
5 tasks
@albertteoh
Copy link
Contributor

I can take a look at this.

@albertteoh
Copy link
Contributor

@jpkrohling is there a way to confirm that this new thrift tag fixes issues #2638 and #2452? I also tried the reproducer in #2638 (comment) but I see the following output:

$ go run main.go
error while processing: *jaeger.Batch error reading struct: *jaeger.Span error reading struct: EOF
memory usage before: 0 MiB
memory usage after: 0 MiB
finished

I was expecting some non-zero memory usage numbers and no error; but maybe I'm wrong?

@Mario-Hofstaetter, @zigmund: Were you able to confirm that changes in #2780 fixed the memory leak issue?

@zigmund
Copy link

zigmund commented Feb 17, 2021

@albertteoh is there docker image containing #2780?
Or I should use jaegertracing/jaeger-agent:latest? Docker hub says it was pushed last time 2 days ago.

@jpkrohling
Copy link
Contributor Author

jaegertracing/jaeger-agent:latest should indeed contain the fix.

@jpkrohling
Copy link
Contributor Author

@jpkrohling is there a way to confirm that this new thrift tag fixes issues #2638 and #2452? I also tried the reproducer in #2638 (comment) but I see the following output:

$ go run main.go
error while processing: *jaeger.Batch error reading struct: *jaeger.Span error reading struct: EOF
memory usage before: 0 MiB
memory usage after: 0 MiB
finished

I was expecting some non-zero memory usage numbers and no error; but maybe I'm wrong?

I would also expect something non-zero, but perhaps it's a rounding error? The message printed before that, about the failure to process the batch, is indicative that the problematic package was received and processing stopped. In unpatched versions, this would trigger the excessive memory consumption. In any case, you can also try the reproducer at https://github.com/Mario-Hofstaetter/jaegertracing-jaeger-issue-2638, by @Mario-Hofstaetter.

@zigmund
Copy link

zigmund commented Feb 17, 2021

@jpkrohling running over hour on jaegertracing/jaeger-agent:latest without memory issues.

update: over 16h running the image, memory consumption as low as 16-17Mb per instance.

Hurray! Waiting for release. Thanks.

@jpkrohling jpkrohling self-assigned this Feb 18, 2021
@jpkrohling
Copy link
Contributor Author

@albertteoh if you don't mind, I'll take this one as I have some free cycles and we could really have a new release soon :-)

@albertteoh
Copy link
Contributor

Please go ahead @jpkrohling :)

@jpkrohling
Copy link
Contributor Author

I'm blocked by https://issues.apache.org/jira/browse/THRIFT-5353. I think we should move forward with the release without this issue here.

@yurishkuro
Copy link
Member

Thrift 0.14.1 was released, presumably fixing https://issues.apache.org/jira/browse/THRIFT-5353. We need to try to upgrade to it.

@yurishkuro yurishkuro changed the title Remove temporary usage of a personal Thrift fork Remove temporary usage of a Thrift fork Mar 3, 2021
jpkrohling added a commit that referenced this issue Mar 4, 2021
Closes #2781

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
albertteoh pushed a commit to albertteoh/jaeger that referenced this issue Mar 5, 2021
Closes jaegertracing#2781

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants