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 Elephantbird #1156

Closed
wants to merge 1 commit into from
Closed

Bump Elephantbird #1156

wants to merge 1 commit into from

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Sep 27, 2023

Not sure if we just should remove it (that would break quite a few APIs, probably something for Parquet 2.0), or update it like this PR. It uses an ancient version:

4.4  -> Thrift 0.7.0 😱
4.17 -> Thrift 0.10.0

>4.4 makes a lot of test helpers private, so I had to remove a few tests.

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@Fokko Fokko requested a review from wgtmac September 27, 2023 20:32
@Fokko Fokko force-pushed the fd-bump-elephantbird branch from 438925a to ad9c067 Compare September 27, 2023 20:59
@wgtmac
Copy link
Member

wgtmac commented Sep 28, 2023

Does an old version of elephantbird prohibit us from upgrading thrift in the future? Even the latest 4.17 is pretty old. Removing these tests may add more risk to break things though I'm not sure whether these modules are still actively used.

@Fokko
Copy link
Contributor Author

Fokko commented Sep 28, 2023

It blocks the upgrade. Somewhere in a static block an exception is being thrown.

Removing these tests may add more risk to break things though I'm not sure whether these modules are still actively used.

The artifact is littered with CVE's, so I hope no-one is using this still.

apache-thrift does not seem to be used anymore (usages of 0 since 1.12.2):
image

apache-protobuf does have some usage:
image

So we can leave that in for now. Let me prepare a PR and also sent out an email on the dev-list.

Fokko added a commit to Fokko/parquet-mr that referenced this pull request Sep 28, 2023
Parquet Thrift relies on Elephantbird:

https://github.com/twitter/elephant-bird

And the current version is still on Thrift 0.7.0.
Updating is non-trivial since a lot of the code
that was used for testing has been made private:

apache#1156

I would suggest removing this from the repository
since it looks like no-on is using it anymore:

`apache-thrift` does not seem to be used anymore (usages of 0 since 1.12.2):
![image](https://github.com/apache/parquet-mr/assets/1134248/526b5358-ac4b-4b3d-82c5-5578ef110418)
@Fokko Fokko mentioned this pull request Sep 28, 2023
4 tasks
@Fokko
Copy link
Contributor Author

Fokko commented Oct 17, 2023

I don't think this is a feasible option, let me close it

@Fokko Fokko closed this Oct 17, 2023
@Fokko Fokko deleted the fd-bump-elephantbird branch October 17, 2023 19:23
@Fokko Fokko restored the fd-bump-elephantbird branch October 20, 2023 17:57
@Fokko Fokko reopened this Oct 20, 2023
Not sure if we just should remove it, or update it like this
PR. It uses an ancient version:

4.4 -> Thrift 0.7.0 😱
4.17 -> Thrift 0.10.0

>4.4 makes a lot of test helpers private, so I had to remove
a few tests.
@Fokko Fokko force-pushed the fd-bump-elephantbird branch 2 times, most recently from 6f444e8 to 6b78b36 Compare October 23, 2023 20:01
@Fokko Fokko closed this Nov 8, 2023
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.

2 participants