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

Use fork of Apache Thrift #2780

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Use fork of Apache Thrift #2780

merged 1 commit into from
Feb 4, 2021

Conversation

jpkrohling
Copy link
Contributor

@jpkrohling jpkrohling commented Feb 3, 2021

Temporarily use a personal fork of Apache Thrift, tagged with 1.13.1, which includes the fix for THRIFT-5322 (apache/thrift#2292).
Fixes #2638 and fixes #2452.
The issue to track the reversion of this PR is #2781.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling jpkrohling requested a review from a team as a code owner February 3, 2021 10:22
@objectiser
Copy link
Contributor

@jpkrohling Would be good to create an issue to revert the change once an official version is available, and link within the description of this PR.

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #2780 (4c9096c) into master (8e5eb9b) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2780      +/-   ##
==========================================
+ Coverage   95.89%   95.90%   +0.01%     
==========================================
  Files         217      217              
  Lines        9625     9625              
==========================================
+ Hits         9230     9231       +1     
  Misses        326      326              
+ Partials       69       68       -1     
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 77.34% <0.00%> (-0.56%) ⬇️
plugin/storage/badger/spanstore/reader.go 96.08% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e5eb9b...dc11c46. Read the comment docs.

@jpkrohling
Copy link
Contributor Author

@Mario-Hofstaetter, @zigmund, would you be open to testing an image with this PR, to confirm that your problems will get fixed?

@Mario-Hofstaetter
Copy link

Mario-Hofstaetter commented Feb 3, 2021

@jpkrohling Will do, hope to get it done by friday.
Is there a download available for a windows binary?

go.sum Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

yurishkuro commented Feb 3, 2021

I thought we wanted the fork to be in github.com/jaegertracing. Why use personal fork?

@jpkrohling
Copy link
Contributor Author

I can push my fork to the jaegertracing organization, but it makes that somewhat too "official". I really want this to be temporary.

@yurishkuro
Copy link
Member

We can reflect that in the forked repo description. Btw pushing to a new repo from yours will not establish the forked link on GitHub.

@zigmund
Copy link

zigmund commented Feb 3, 2021

@jpkrohling yes, I can test image in few days.

@jpkrohling
Copy link
Contributor Author

Btw pushing to a new repo from yours will not establish the forked link on GitHub

Yes, sorry, I meant that I would fork Apache Thrift to jaegertracing org and push the tag there.

@jpkrohling
Copy link
Contributor Author

PR updated to use the fork from the jaegertracing organization.

Temporarily use a personal fork of Apache Thrift, tagged with 1.13.1, which includes the fix for THRIFT-5322 (apache/thrift#2292).
Fixes #2638 and #2452.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@yurishkuro yurishkuro merged commit c7c371d into jaegertracing:master Feb 4, 2021
@jpkrohling jpkrohling deleted the jpkrohling/use-fork-of-thrift branch July 28, 2021 19:21
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.

jaeger-agent reproducible memory leak in 1.21.0 Memory peaks on agent v1.19.2
5 participants