-
Notifications
You must be signed in to change notification settings - Fork 36
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
HADOOP-17679. Upgrade Protobuf to 3.17.3 #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Makes sense for hadoop-thirdparty 1.2.0.
@medb could you push an empty commit to trigger the precommit test?
So at the end of the day, we probably don't need to use protobuf in the hadoop-thirdparty repository. See: #14 (comment) |
Instead, we should do this for the main hadoop repository, not the hadoop-thirdparty repo. https://github.com/apache/hadoop/blob/trunk/dev-support/docker/Dockerfile#L120 |
@jojochuang May you clarify, do you mean that we do not need to compile protoc in |
Yeah. We have a number of redundant steps in hadoop-thirdparty Dockerfile. It was copied from the main hadoop repo but we don't need many of the packages. |
@jojochuang rebased on the trunk HEAD and updated PR to the latest Protobuf version |
As I said, this PR is not needed. I'd like to ask you to work on the protobuf update in the main hadoop repository. |
@jojochuang sent PR to the Hadoop repo: apache/hadoop#3384 Please, let me know if this is correct. |
@jojochuang I took a look at Hadoop changes in apache/hadoop#3384, and it seems that to upgrade Protobuf both PRs are required (#13 in May you explain, how can I update Protobuf without any changes in Hadoop Thirdparty repository (i.e. this PR)? |
It would be good to update this to a non-vulnerable version of protobuf |
Superseded by #38. |
Protobuf 3.17.3 is the latest Protobuf release now.