-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
WIP: HBASE-28883 Consume hbase-thirdparty-bom and cleanup impacted depende… #6366
base: master
Are you sure you want to change the base?
Conversation
0b51c71
to
f795fc0
Compare
This is a demo PR and is bound to fail to compile as it needs snapshot version of hbase-thirdparty. To test this we need to first build hbase -thirdparty apache/hbase-thirdparty#124 locally and this one next. CC: @ndimiduk |
pom.xml
Outdated
hbase-thirdparty. | ||
<hbase-thirdparty.version>4.1.10-SNAPSHOT</hbase-thirdparty.version> | ||
<!--Version of protobuf that hbase uses internally (we shade our pb) | ||
Must match what is out in hbase-thirdparty include. |
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.
replace with:
"Version of protobuf that hbase uses internally (we shade our pb).
Must be kept in sync with the version of protobuf shipped in hbase-thirdparty"
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.
Why do we need to keep internal.protobuf.version
at all?
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.
Could not remove the hardcoding of internal.protobuf.version as with BOM seems its not possible to pass on properties. And we actually use the protobuf version property in our code in protobuf-maven-plugin
plugin
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.
That's super annoying.
Okay but maybe there's a hack. Can we use the bundle and process goals from https://maven.apache.org/plugins/maven-remote-resources-plugin/index.html to package up this property and then import it from the hbase-protocol-shaded build via https://www.mojohaus.org/properties-maven-plugin/read-project-properties-mojo.html ?
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.
Sure let me explore this and come back to you
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.
Sorry Nick for the delay here, got caught up with other tasks. Will come back and post an update soon!
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.
That's super annoying.
Okay but maybe there's a hack. Can we use the bundle and process goals from https://maven.apache.org/plugins/maven-remote-resources-plugin/index.html to package up this property and then import it from the hbase-protocol-shaded build via https://www.mojohaus.org/properties-maven-plugin/read-project-properties-mojo.html ?
I tried this approach. Unfortunately this also did not work as "Properties used in plugin configurations must be available during the POM parsing phase."
We can read/inject a property only after parsing is done; So the compile-protoc
would still build with whatever is set as initial value for internal.protobuf.version
in pom.xml
Approach:
Create a new module with hbase-thirdparty-properties-bundle/pom.xml
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.hbase.thirdparty</groupId>
<artifactId>hbase-thirdparty</artifactId>
<version>${revision}</version>
<relativePath>..</relativePath>
</parent>
<artifactId>hbase-thirdparty-properties-bundle</artifactId>
<name>Apache HBase Thirdparty - Properties Bundle</name>
<description>
Properties Bundle, for shared properties between HBase Thirdparty and HBase main code
</description>
<build>
<resources>
<resource>
<directory>src/main/resources</directory>
<filtering>true</filtering>
</resource>
</resources>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-resources-plugin</artifactId>
<executions>
<execution>
<id>filter-resources</id>
<phase>process-resources</phase>
<goals>
<goal>resources</goal>
</goals>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
Create a properties to be shared: hbase-thirdparty-properties-bundle/src/main/resources/dependencies.properties
internal.protobuf.version=${protobuf.version}
Define a dummy value for
<internal.protobuf.version>999.999.999</internal.protobuf.version>
so that we can still parse the pom.xml
Next, add a plugin to consume/load the property from resource in hbase/pom.xml
<!-- Plugins to unpack properties file from hbase-thirdparty -->
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<id>unpack</id>
<phase>initialize</phase>
<goals>
<goal>unpack</goal>
</goals>
<configuration>
<artifactItems>
<artifactItem>
<groupId>org.apache.hbase.thirdparty</groupId>
<artifactId>hbase-thirdparty-properties-bundle</artifactId>
<version>${hbase-thirdparty.version}</version>
<overWrite>true</overWrite>
<!-- Specify the output directory -->
<outputDirectory>${project.build.directory}/extracted-resources</outputDirectory>
</artifactItem>
</artifactItems>
</configuration>
</execution>
</executions>
</plugin>
<!-- Properties Maven Plugin to read properties file unpacked via maven-dependency-plugin -->
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>properties-maven-plugin</artifactId>
<version>1.2.1</version>
<executions>
<execution>
<id>read-properties</id>
<phase>initialize</phase>
<goals>
<goal>read-project-properties</goal>
</goals>
<configuration>
<files>
<file>${project.build.directory}/extracted-resources/dependencies.properties</file>
</files>
</configuration>
</execution>
</executions>
</plugin>
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.
Maven load project model (from pom.xml) for project and all submodules at startup time. During project reading all available properties are resolved from projects and settings properties.
This plugin (as all other) is executed in the later phase - when project model is already build in memory.
So properties read from external files by this plugin can not by used in project definitions in items like , and so on.
https://www.mojohaus.org/properties-maven-plugin/#plugin-limitations
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Nice. How do we provie to ourselves that this change is correct? A diff of before and after dependency:list of each module?
<artifactId>netty-bom</artifactId> | ||
<version>${netty4.version}</version> | ||
<type>pom</type> | ||
<scope>import</scope> |
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.
Does importing boms work this way? we can get transitivity over a series of imports?
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.
Yes it seems to work!
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.
Proof added at #6366 (comment)
pom.xml
Outdated
hbase-thirdparty. | ||
<hbase-thirdparty.version>4.1.10-SNAPSHOT</hbase-thirdparty.version> | ||
<!--Version of protobuf that hbase uses internally (we shade our pb) | ||
Must match what is out in hbase-thirdparty include. |
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.
Why do we need to keep internal.protobuf.version
at all?
Sure, let me share the dependency tree with and without this change. |
mvn dependency:list
mvn dependency:tree
Skimmed the files, looks fine except the fact that maybe we should not manage Expected: [INFO] com.google.protobuf:protobuf-java:jar:2.5.0:compile -- module protobuf.java (auto)
We have netty version changes as both are not in sync in master and hbase-thirdparty and this also clears the transitivity question asked earlier. |
f795fc0
to
e0d9077
Compare
…ncies