voonhous commented on code in PR #19596:
URL: https://github.com/apache/hudi/pull/19596#discussion_r3772923046
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -124,6 +125,29 @@ public class HoodieAvroUtils {
private static final Properties PROPERTIES = new Properties();
+ /**
+ * Resolves the Avro library version, preferring Maven's generated
pom.properties over
+ * {@link Package#getImplementationVersion()}. The latter reads the jar
manifest, which is
+ * present for a standalone avro-*.jar but is usually dropped for the avro
package once its
+ * classes are merged into a shaded/fat jar.
+ */
+ private static String resolveAvroVersion() {
+ String avroPomPropertiesPath =
"META-INF/maven/org.apache.avro/avro/pom.properties";
+ try (InputStream inputStream =
Schema.class.getClassLoader().getResourceAsStream(avroPomPropertiesPath)) {
Review Comment:
Blocker: this lookup is not anchored to the jar that actually defines
`Schema`, so it can report the version of an Avro that is not the one executing.
`ClassLoader.getResourceAsStream` returns the first hit in
classpath/delegation order. maven-shade relocates the Avro *classes* but leaves
`META-INF/maven/org.apache.avro/avro/pom.properties` at the original
coordinate, so a different jar can answer the query. Checked against the built
bundles: `hudi-hadoop-mr-bundle`, `hudi-presto-bundle`, `hudi-trino-bundle`,
`hudi-flink1.20-bundle` and `hudi-timeline-server-bundle` each contain
`org/apache/hudi/org/apache/avro/Schema.class` plus an unrelocated
`META-INF/maven/org.apache.avro/avro/pom.properties`.
Reproduced on the standard Hive layout (hive-exec on the parent loader,
hudi-hadoop-mr-bundle on a child loader, which is what `ADD JAR` /
`hive.aux.jars.path` produce):
```
loaded class : org.apache.hudi.org.apache.avro.Schema
codeSource (jar) : hudi-hadoop-mr-bundle-1.3.0-SNAPSHOT.jar <- avro 1.11.4
is what executes
OLD implVersion : 1.3.0-SNAPSHOT
NEW resolved ver : 1.8.2
resource URL won :
jar:file:.../hive-exec-3.1.3.jar!/META-INF/maven/org.apache.avro/avro/pom.properties
```
`hive-exec` 3.1.3 and 2.3.10 both ship that resource with `version=1.8.2`.
This is not an exotic classpath. Other jars that ship that exact resource while
shipping no `org/apache/avro/Schema.class` at all: `hadoop-client-runtime`
3.4.2 and 3.4.3 (claims 1.11.4, classes relocated under
`org/apache/hadoop/shaded/`), `pulsar-spark-connector_2.12` (1.8.2) and `_2.13`
(1.10.2) which is a `provided` dependency of hudi-utilities, `pulsar-client-*`,
and `iceberg-flink-runtime-1.19-1.6.1`.
Please bind the lookup to the archive that defines `Schema` before falling
back:
```java
private static String resolveAvroVersion() {
final String path = "META-INF/maven/org.apache.avro/avro/pom.properties";
try {
URL schemaClassUrl = Schema.class.getResource("Schema.class");
String schemaArchive = schemaClassUrl == null ? null :
archiveOf(schemaClassUrl);
Enumeration<URL> candidates =
Schema.class.getClassLoader().getResources(path);
while (candidates.hasMoreElements()) {
URL candidate = candidates.nextElement();
// only trust a pom.properties that ships in the same archive as the
loaded Schema class
if (schemaArchive != null &&
!schemaArchive.equals(archiveOf(candidate))) {
continue;
}
Properties avroProperties = new Properties();
try (InputStream in = candidate.openStream()) {
avroProperties.load(in);
}
String version = avroProperties.getProperty("version");
if (version != null) {
return version;
}
}
} catch (Exception e) {
LOG.warn("Failed to resolve the avro version from {}, falling back to
the jar manifest", path, e);
}
return Schema.class.getPackage() == null ? null :
Schema.class.getPackage().getImplementationVersion();
}
private static String archiveOf(URL url) {
String s = url.toString();
int bang = s.indexOf("!/");
return bang < 0 ? s : s.substring(0, bang);
}
```
This needs imports for `java.net.URL` and `java.util.Enumeration`, and a
`private static final Logger LOG =
LoggerFactory.getLogger(HoodieAvroUtils.class);` field since `HoodieAvroUtils`
has no logger today. The `LOG.warn` also covers the swallowed-exception point
raised earlier in this review.
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -124,6 +125,29 @@ public class HoodieAvroUtils {
private static final Properties PROPERTIES = new Properties();
+ /**
+ * Resolves the Avro library version, preferring Maven's generated
pom.properties over
+ * {@link Package#getImplementationVersion()}. The latter reads the jar
manifest, which is
+ * present for a standalone avro-*.jar but is usually dropped for the avro
package once its
+ * classes are merged into a shaded/fat jar.
Review Comment:
The premise in this javadoc is not correct, and fixing it matters because
the real defect is worse than the one described.
The manifest entry is not "dropped" when avro is merged into a fat jar.
`URLClassLoader.definePackage` falls back to the uber jar's main manifest
attributes, so the avro package inherits the *assembling* jar's version.
Checked against the built bundles:
```
$ unzip -p hudi-hive-sync-bundle-1.3.0-SNAPSHOT.jar META-INF/MANIFEST.MF |
grep Implementation-Version
Implementation-Version: 1.3.0-SNAPSHOT
$ unzip -p hudi-hive-sync-bundle-1.3.0-SNAPSHOT.jar
META-INF/maven/org.apache.avro/avro/pom.properties
version=1.12.1
```
So `AVRO_VERSION` is `"1.3.0-SNAPSHOT"` in that bundle today, and
`gteqAvro1_12()` returns false while the bundle ships avro 1.12.1. That is a
real pre-existing bug that this PR fixes, and it is a stronger motivation than
the null NPE. Same shape in hudi-presto-bundle, hudi-trino-bundle and
hudi-timeline-server-bundle (all avro 1.12.1), and in hudi-hadoop-mr-bundle and
hudi-flink1.20-bundle (avro 1.11.4).
Two asks:
1. Reword the javadoc along the lines of "the manifest belongs to the
assembling jar, so once avro is merged or relocated it reports that jar's
version rather than avro's, or none at all".
2. `gteqAvro1_12()` flips false -> true in the hive-sync / presto / trino /
timeline-server bundles, which changes
`convertDefaultValueForAvroCompatibility` behaviour in shipped artifacts.
Please update "Risk Level: low" in the PR description to say so, and run bundle
validation on at least one avro-merging bundle before merge.
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -112,7 +113,7 @@
*/
public class HoodieAvroUtils {
- public static final String AVRO_VERSION =
Schema.class.getPackage().getImplementationVersion();
+ public static final String AVRO_VERSION = resolveAvroVersion();
Review Comment:
Backport ask: the reporter on #19595 is on 1.2.x, and their stack trace
shows the pre-rename `org.apache.hudi.avro.HoodieAvroUtils` package. The defect
is present unchanged on the release branch:
```
$ git show
release-1.2.0:hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java
| grep -n 'AVRO_VERSION = '
109: public static final String AVRO_VERSION =
Schema.class.getPackage().getImplementationVersion();
```
(verified against both `refs/heads/release-1.2.0` at `f05c83f2b977` and the
`release-1.2.0` tag)
A master-only fix lands nothing for the reporter. Please confirm whether a
`release-1.2.0` backport is planned and note it in the PR description either
way.
##########
hudi-hadoop-common/src/main/java/org/apache/parquet/avro/AvroSchemaConverterWithTimestampNTZ.java:
##########
@@ -669,15 +669,11 @@ private static String appendPath(String path, String
fieldName) {
/* Avro <= 1.9 does not support conversions to LocalTimestamp{Micros,
Millis} classes */
private static boolean avroVersionSupportsLocalTimestampTypes() {
- final String avroVersion = getRuntimeAvroVersion();
+ final String avroVersion = HoodieAvroUtils.AVRO_VERSION;
return avroVersion == null
|| !(avroVersion.startsWith("1.7.")
|| avroVersion.startsWith("1.8.")
|| avroVersion.startsWith("1.9."));
}
Review Comment:
This one-line refactor is what turns the classloader issue in the other
comment into a data-semantics bug, and it is not needed to fix the NPE.
Before this PR, `AVRO_VERSION` had exactly one production consumer:
`gteqAvro1_12()` feeding `convertDefaultValueForAvroCompatibility`
(`HoodieAvroUtils.java:1570`). `gteqAvro1_9()` and `gteqAvro1_10()` are public
but have no non-test callers. This change adds a second consumer, and it is the
timestamp one.
In the Hive reproduction from the other comment:
```
avroVersionSupportsLocalTimestampTypes() [OLD, via Schema.Parser manifest] =
true
avroVersionSupportsLocalTimestampTypes() [NEW, via AVRO_VERSION] =
false
```
The old local probe returned the bundle's own `1.3.0-SNAPSHOT`, which is
garbage but happens to satisfy the `!startsWith("1.7."/"1.8."/"1.9.")`
predicate. Routing through `AVRO_VERSION` yields `1.8.2` and flips it. At line
574 that sends a parquet `TIMESTAMP(isAdjustedToUTC=false)` column down the
`createTimestampMillis()`/`createTimestampMicros()` branch instead of
`createLocalTimestampMillis()`/`Micros()`, i.e. a TIMESTAMP_NTZ column silently
reads back as a UTC-adjusted instant. The path is live in that bundle:
`HoodieAvroParquetReader` -> `HoodieAvroParquetSchemaConverter` -> this class,
all three present in `hudi-hadoop-mr-bundle-1.3.0-SNAPSHOT.jar`.
The PR describes this change as "to keep the consistent behavior", so it is
a pure refactor carrying all of the risk and none of the fix. Please drop it
from this PR and keep the local probe:
```suggestion
private static boolean avroVersionSupportsLocalTimestampTypes() {
final String avroVersion = getRuntimeAvroVersion();
return avroVersion == null
|| !(avroVersion.startsWith("1.7.")
|| avroVersion.startsWith("1.8.")
|| avroVersion.startsWith("1.9."));
}
private static String getRuntimeAvroVersion() {
return Schema.Parser.class.getPackage().getImplementationVersion();
}
```
(the `org.apache.avro.Schema` import and the `HoodieAvroUtils` import need
reverting too)
If you do want to dedupe the two probes in a follow-up, use a feature check
rather than a version check - it is classloader-consistent by construction and
immune to the whole problem:
```java
private static boolean avroVersionSupportsLocalTimestampTypes() {
try {
LogicalTypes.localTimestampMillis(); // added in avro 1.10, absent in
1.7 - 1.9
return true;
} catch (NoSuchMethodError e) {
return false;
}
}
```
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -124,6 +125,29 @@ public class HoodieAvroUtils {
private static final Properties PROPERTIES = new Properties();
+ /**
+ * Resolves the Avro library version, preferring Maven's generated
pom.properties over
+ * {@link Package#getImplementationVersion()}. The latter reads the jar
manifest, which is
+ * present for a standalone avro-*.jar but is usually dropped for the avro
package once its
+ * classes are merged into a shaded/fat jar.
+ */
+ private static String resolveAvroVersion() {
Review Comment:
No test covers the new resolver, and CI cannot catch a regression here as
things stand. `avro.version` is pinned to 1.11.4 in every profile
(`pom.xml:184`, plus the profile overrides at `:2660`, `:2723`, `:2770`), so
the `1.7`/`1.8`/`1.9` branch of `avroVersionSupportsLocalTimestampTypes()` is
never exercised, and `packaging/bundle-validation/` does not touch version
resolution or timestamp-NTZ. Codecov reports 5 uncovered lines here for the
same reason.
A cheap discriminating assertion needs no refactor: under surefire, avro is
a plain jar carrying both the manifest and the pom.properties, so the two
sources must agree. Please add to
`hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java`:
```java
@Test
void testAvroVersionMatchesLoadedAvroJar() {
assertNotNull(HoodieAvroUtils.AVRO_VERSION);
// the pom.properties lookup must agree with the jar that actually defines
Schema
assertEquals(Schema.class.getPackage().getImplementationVersion(),
HoodieAvroUtils.AVRO_VERSION);
}
```
That fails exactly when the resource lookup drifts from the loaded avro,
which is the regression in the other comment.
If you also want the shaded-jar branch covered, there is an in-repo
precedent for this exact pattern: `HoodieVersion` reads
`META-INF/maven/org.apache.hudi/hudi-common/pom.properties` the same way
(`hudi-common/src/main/java/org/apache/hudi/HoodieVersion.java:59`) and made
itself testable with `setVersionOverride` at `:114`, used throughout
`hudi-common/src/test/java/org/apache/hudi/TestHoodieVersion.java:29`. Worth
following that here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
