-
Notifications
You must be signed in to change notification settings - Fork 424
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
TEZ-4363: Bump protobuf dependency to 3.x #192
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -90,7 +91,7 @@ public void toProtoStream(CodedOutputStream outputStream) throws IOException { | |||
|
|||
@Override | |||
public void fromProtoStream(CodedInputStream inputStream) throws IOException { | |||
AMLaunchedProto proto = inputStream.readMessage(AMLaunchedProto.PARSER, null); | |||
AMLaunchedProto proto = inputStream.readMessage(AMLaunchedProto.parser(), ExtensionRegistry.newInstance()); |
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.
- is this PARSER -> parser() change backward compatible?
- what is ExtensionRegistry.newInstance() about exactly?
in general, I want to avoid every direct change in the first place which makes tez unable to compile on 2.5
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.
- The
PARSER
field is deprecated in version 3 but replacing it withparser()
is breaking backward compatibility so this needs to be reverted. - Protobuf3 introduced a nullcheck on the
extensionRegistry
parameter so if it gets null value then it will throw aNullpointerException
, that’s why I replaced it withExtensionRegistry.newInstance()
which is creating an empty instance (after checking thisgetEmptyRegistry()
would be better since it is creating empty maps for the new instance)
I checked the generated code both on Protobuf2 and Protobuf3 but it seems thatextensionRegistry
param is not used in our inputStream.readMessage use case.
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.
thanks @mbathori-cloudera, I think now we can move on with PARSER (even if it's deprecated), as we haven't decided about a major tez release yet, so backward compatibility now looks more important to me
@mbathori-cloudera @abstractdog |
by default, I wanted to merge the benchmark ticket first (TEZ-4362), but considering that the world is moving towards protobuf 3, I believe it's fine to merge this patch first and then do the perf benchmark later |
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
@abstractdog I would need some help with the Yetus build. I’ve changed the |
@mbathori-cloudera, I was able to reproduce, maybe this is related to TEZ-4300, not sure at the moment
I believe the issue might be related to the automatically downloaded protobuf (under build-tools/protobuf), I'm suspecting that if you build from a folder of a submodule, it's not downloaded automatically before building and still has only 2.5.0 present, hence the failure:
|
@abstractdog thanks I could reproduce the issue with trying to compile only the submodule. It seems like the protobuf download script is not triggered from the submodules and the |
@mbathori-cloudera Any updates on this PR. Can we prioritize this fix. |
hey all! I forgot to review @mbathori-cloudera 's last comment so regarding:
I don't mind if we have to add this to every submodule if it works only this way, as we want to be able to build submodules alone in their subfolders too
also, I'm wondering if it's enough to define the plugin only once in root pom.xml's pluginManagement and re-declare only "some parts" of it in submodules (so exploit maven's ability to inherit plugins) |
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
@abstractdog I’ve added the protobuf install plugin to the required submodules and now they can be built independently on localhost however the PR build is still getting Another issue seems to be that wget is missing from the environment |
okay, so we have different problems, let me think about the stuck 2.5.0 version |
Sure I'm creating a ticket for the wget issue. |
I think I found the problem
|
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
thanks @abstractdog for checking it, I've updated the checkstyle issues and as I can see the wget patch was merged so now this issue seems to be fixed based on the logs however another problem occured: Should I make a ticket and PR for this missing sudo command? |
would it be possible to use latest 3.21.1 version? it contains artefact for Mac M1 aarm64 we are doing similar upgrade in HIVE apache/hive#3309 and to compile .proto with maven we use https://github.com/os72/protoc-jar-maven-plugin (ie no need to have .sh to download protoc)
|
yes, please
here, you might try to attach your tez folder to docker's home I guess |
yes, thanks @slachiewicz, 3.21.1 makes sense, let's do it in this PR (cc: @mbathori-cloudera) regarding protoc-jar-maven-plugin, currently we use this for building protos: Lines 170 to 198 in 24a77c9
I guess we can move to protoc-jar-maven-plugin the .sh for downloading protoc is different than generating the sources I think |
@abstractdog I've built and tested locally the docker image and I've managed to run the protobuf install script after adding sudo, so it seems like it was the last missing command. I've made a ticket and a PR for this change: TEZ-4421 I also bumped the protobuf version to 3.21.1 which needed some further fixing in the install script since they removed the major version number from the file names e.g:
I'll update this PR after TEZ-4421 gets merged. |
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
(!) A patch to the testing environment has been detected. |
This comment was marked as outdated.
This comment was marked as outdated.
@abstractdog I replaced the protobuf install and compile plugin with protoc-jar-maven-plugin as slachiewicz recommended it. It handles the plugin download and compilation so I think we can get rid of the |
protoc-jar-maven-plugin absolutely makes sense to me, however I would like to see it in a different PR, as it's not related to protobuf 3.x upgrade, could you please take care of it @mbathori-cloudera? |
@abstractdog I've created a ticket for the protobuf plugin change (TEZ-4428) and a pull request for it: #218 |
@mbathori-cloudera: could you update this PR with the latest patch? please squash commits together for easier reference, thanks! |
(!) A patch to the testing environment has been detected. |
💔 -1 overall
This message was automatically generated. |
@abstractdog @mark-bathori Has this change completed or are there further patches that need to be done. Please let me know if I can also help in getting this done. |
@abstractdog @mark-bathori Are there any blockers for this. This has been idle for over couple of months |
checked locally, test failures are only because of the nature of precommit: we can change protobuf version on-the-fly locally by -Dprotobuf.version, also can change the protobuf version in yetus docker image, but we don't guarantee they're in sync so failures should not happen when this patch is committed to master what I checked locally, all passed:
|
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.
LGTM +1
@abstractdog LGTM, +1. Can we merge these changes ? |
…Bathori reviewed by Laszlo Bodor, Aman Raj)" This reverts commit c386865.
No description provided.