HADOOP-16558. [COMMON+HDFS] use protobuf-maven-plugin to generate protobuf classes #1494
Conversation
…rate protobuf classes
…te protobuf classes
vinayakumarb
force-pushed
the
HADOOP-13363/HADOOP-16558
branch
from
9ef3e39
to
f72cfca
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Test failures are unrelated. shaded-client failures are due to protoc 2.5.0 missing in Docker image. This run didnt pickup the commit of HADOOP-16589. Next run https://builds.apache.org/job/hadoop-multibranch/job/PR-1494/2/artifact/out/patch-shadedclient.txt passed for shaded client verification, but next unfortunately took more time and jenkins aborted it. |
| <protoSourceRoot>${basedir}/src/main/proto</protoSourceRoot> | ||
| <outputDirectory>${project.build.directory}/generated-sources/java</outputDirectory> | ||
| <clearOutputDirectory>false</clearOutputDirectory> | ||
| <skip>true</skip> |
There was a problem hiding this comment.
Do we need this? I suppose this is under the PluginMangement tag, so unless we declare this plugin explicitly in the pom of a sub module, it will not be executed?
And maybe we can add the test complie execution here with the skip = true configuration, and in the sub module which also need to generate test protos can use skip = false to enable the execution.
There was a problem hiding this comment.
Do we need this? I suppose this is under the PluginMangement tag, so unless we declare this plugin explicitly in the pom of a sub module, it will not be executed?
Yes! you are right. Need to add plugin and execution with "skip=false" explicitly in submodule to enable this. The reason for keeping in plugin management is, all other configuration items could be controlled from one place for all modules. Whatever needs to be specific to submodule, could be controlled in specific module.
And maybe we can add the test complie execution here with the skip = true configuration, and in the sub module which also need to generate test protos can use skip = false to enable the execution.
Yes you are right. I will do this change.
| <skip>false</skip> | ||
| <additionalProtoPathElements> | ||
| <additionalProtoPathElement> | ||
| ${basedir}/../../hadoop-common-project/hadoop-common/src/main/proto |
There was a problem hiding this comment.
If we also attach the proto along in the jar, we do not need to add this configuration. This may change the behavior but I think it is fine? Of course, keep the old way is also fine, not a big problem. Just want to make the pom simpler. As in the old way, you need make sure that we also depend on hadoop-common in the dependencies section, otherwise the protoc generating is fine but there will be compile error...
There was a problem hiding this comment.
Yes. I agree that keeping proto files in jar may be fine for now.
It would be better to specify explicitly which proto files are required for successfull proto generation. So I preferred to NOT to include in jars.
As in the old way, you need make sure that we also depend on hadoop-common in the dependencies section, otherwise the protoc generating is fine but there will be compile error...
hadoop-common in dependency section is anyway required, irrespective of whether proto files are part of jar or not.
…rate protobuf classes. Moved test-compile-protoc to hadoop-project/pom.xml
|
💔 -1 overall
This message was automatically generated. |
hadoop-project/pom.xml
Outdated
| </configuration> | ||
| </execution> | ||
| <execution> | ||
| <id>test-compile-protoc</id> |
There was a problem hiding this comment.
nit: let's align the names? Either we change this to src-test-compile-protoc, or we change the above to compile-protoc, I know that this will introduce conflicts with the plugin defined in yarn-csi, but let's do things cleanly?
There was a problem hiding this comment.
Okay, I will update id as per your suggestion, "src-test-compile-protoc" and merge :)
There was a problem hiding this comment.
Thanks. Will rebase #1496 after you merge this.
|
Test failures are not related. Most of them are due to "Unable to create native thread" |
|
Merged. Thanks @Apache9 for the reviews. |
|
💔 -1 overall
This message was automatically generated. |
…tobuf classes (apache#1494). Contributed by Vinayakumar B.
…tobuf classes (apache#1494). Contributed by Vinayakumar B.
Use "protoc-maven-plugin" to dynamically download protobuf executable to generate protobuf classes from proto files.