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

Bump Apache Thrift to 0.10.0 #8419

Merged
merged 10 commits into from
Nov 5, 2019
Merged

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Aug 28, 2019

Bump Apache Thrift to at least 0.10.0 for now. https://github.com/apache/thrift/blob/master/CHANGES.md#0100

Description


This PR has:

  • been self-reviewed.

@Fokko
Copy link
Contributor Author

Fokko commented Sep 13, 2019

Rebased onto master

@jihoonson
Copy link
Contributor

+1 after CI.

LGTM and Teamcity are failing because of the missing libthrift-0.5.0-1 jar. I think we can solve this by simply updating the version of scrooge-maven-plugin which is using that versino of libthrift.

This PR will make Travis failed because it's not updating licences.yaml. I will raise a PR to fix that shortly.

@Fokko
Copy link
Contributor Author

Fokko commented Nov 5, 2019

scrooge-maven-plugin is already using Thrift 0.10.0. The old version 4.11.0 of scrooge-maven-plugin is indeed using libthrift-0.5.0-1 jar. I've rebased onto master.

Why is this a development blocker?

@clintropolis
Copy link
Member

Why is this a development blocker?

The old version of scrooge-maven-plugin uses a super old forked version of libthrift for some reason that isn't on maven central, and seems to have recently stopped working for teamcity and lgtm CI checks, meaning no PRs are passing.

@clintropolis
Copy link
Member

clintropolis commented Nov 5, 2019

This PR will make Travis failed because it's not updating licences.yaml.

It actually might not, the old version doesn't appear to be in licenses.yaml, though maybe it should be since there is a NOTICE file, https://github.com/apache/thrift/blob/master/NOTICE.

@Fokko
Copy link
Contributor Author

Fokko commented Nov 5, 2019

Thanks, updating scrooge should fix the dependency on the ancient version of libthrift indeed. Jenkins is running on my fork as well: https://travis-ci.org/Fokko/druid/builds/607854795 The one on Apache is still pending due to capacity issues.

@clintropolis
Copy link
Member

though maybe it should be since there is a NOTICE file, https://github.com/apache/thrift/blob/master/NOTICE.

Responding to myself, but I don't think we need to worry about updating licenses.yaml in this PR.

@jihoonson
Copy link
Contributor

The LGTM is failing with the same error even after I restarted once. Since TeamCity looks fine, I think it's an issue with LGTM (maybe it's caching something). I'll merge this PR shortly and will see other builds succeed.

@jihoonson jihoonson merged commit 3b602da into apache:master Nov 5, 2019
@Fokko Fokko deleted the fd-bump-to-thrift-0-10-0 branch November 6, 2019 16:36
clintropolis pushed a commit to implydata/druid-public that referenced this pull request Nov 15, 2019
* Bump Apache Thrift to 0.10.0

* Remove unused dependency

* Bump maven-scrooge-plugin to the latest
imply-rjenkins pushed a commit to implydata/druid-public that referenced this pull request Nov 21, 2019
* Bump Apache Thrift to 0.10.0

* Remove unused dependency

* Bump maven-scrooge-plugin to the latest
jon-wei pushed a commit to jon-wei/druid that referenced this pull request Nov 27, 2019
* Bump Apache Thrift to 0.10.0

* Remove unused dependency

* Bump maven-scrooge-plugin to the latest
jon-wei added a commit that referenced this pull request Nov 27, 2019
* Bump Apache Thrift to 0.10.0

* Remove unused dependency

* Bump maven-scrooge-plugin to the latest
Thomas-Laird pushed a commit to Thomas-Laird/druid that referenced this pull request Dec 3, 2019
* Bump Apache Thrift to 0.10.0

* Remove unused dependency

* Bump maven-scrooge-plugin to the latest
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants