hudi-agent commented on code in PR #19596:
URL: https://github.com/apache/hudi/pull/19596#discussion_r3776032849
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -124,6 +129,52 @@ 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 comes from
whatever manifest happens to
+ * seal the package, which is only avro's own manifest when avro ships as a
standalone jar. But once
+ * its classes get merged or relocated into a shaded/fat jar, that lookup
silently returns the
+ * assembling jar's version instead of avro's or nothing at all. So better
to resolve with pom.properties
+ * followed by manifest version.
+ */
+ 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 use the 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);
+ }
+ String manifestVersion = Schema.class.getPackage() == null ? null :
Schema.class.getPackage().getImplementationVersion();
+ if (manifestVersion == null) {
+ log.warn("Could not resolve the avro version from {} nor from the jar
manifest, "
+ + "avro version checks will fall back to pre-1.9 behaviour.
Check that avro jar is on the classpath.", path);
+ }
+ return manifestVersion;
+ }
+
+ private static String archiveOf(URL url) {
+ String s = url.toString();
Review Comment:
π€ nit: could you rename `s` to `urlString` (or `urlStr`)? Single-letter
variable names in short methods are fine for counters, but here it's the
method's whole raison d'Γͺtre and a more descriptive name would make the
subsequent `indexOf` / `substring` chain self-explanatory at a glance.
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java:
##########
@@ -1191,4 +1192,14 @@ public void testConvertToRecord() throws IOException {
assertEquals(NUM_FIELDS_IN_EXAMPLE_SCHEMA,
stripped.getSchema().getFields().size());
assertEquals("key1", stripped.get("_row_key").toString());
}
+
+ /**
+ * Verifies whether loaded avro version from pom.properties matches with
implementation version from manifest
+ */
+ @Test
+ void testAvroVersionMatchesLoadedAvroJar() {
+ assertNotNull(HoodieAvroUtils.AVRO_VERSION);
+ // the pom.properties lookup must agree with the jar that actually defines
Schema
Review Comment:
π€ nit: the test asserts equality with
`Schema.class.getPackage().getImplementationVersion()`, but that's exactly the
old fallback path β so in a shaded-jar environment (the scenario this PR is
fixing) the assertion would fail. It might be worth guarding with
`assumeTrue(manifestVersion != null)` or just asserting `assertNotNull` only,
leaving a comment that the pom.properties path is tested implicitly by the
non-null check.
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
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:
π€ The PR description does already state a backport is intended ("Will
backport this to release-1.2 branch as well"), so that part looks covered β
though it'd be worth @Rajeev-01 confirming the exact target (release-1.2.0,
which is where you verified the pre-rename
`org.apache.hudi.avro.HoodieAvroUtils` defect) so the reporter on 1.2.x
actually gets the fix.
--
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]
