hudi-agent commented on code in PR #19596:
URL: https://github.com/apache/hudi/pull/19596#discussion_r3774218981
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -124,6 +129,49 @@ public class HoodieAvroUtils {
private static final Properties PROPERTIES = new Properties();
+ private static final Logger LOG =
LoggerFactory.getLogger(HoodieAvroUtils.class);
+
+ /**
+ * 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);
Review Comment:
🤖 `LOG` (line 132) is declared after `AVRO_VERSION` (line 120), and static
fields initialize in textual order — so `AVRO_VERSION = resolveAvroVersion()`
runs while `LOG` is still null. If the try block throws (e.g.
`getResources`/`openStream` IOException), this `LOG.warn(...)` would NPE inside
the catch and surface as an `ExceptionInInitializerError` — arguably worse than
the original NPE this PR is fixing. Could you move the `LOG` declaration above
`AVRO_VERSION`?
<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:
##########
@@ -124,6 +129,49 @@ public class HoodieAvroUtils {
private static final Properties PROPERTIES = new Properties();
+ private static final Logger LOG =
LoggerFactory.getLogger(HoodieAvroUtils.class);
+
+ /**
+ * 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);
+ }
+ return Schema.class.getPackage() == null ? null :
Schema.class.getPackage().getImplementationVersion();
+ }
+
+ private static String archiveOf(URL url) {
+ String s = url.toString();
Review Comment:
🤖 nit: `bang` is familiar in shell/Unix circles but might read as jargon to
Java developers who haven't seen `!` called that before. Could you rename it to
`separatorIdx` or `jarSeparatorIdx` to make it self-evident without needing the
mental translation?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
