voonhous commented on code in PR #19596:
URL: https://github.com/apache/hudi/pull/19596#discussion_r3774618510
##########
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:
Thanks, the anchoring is what I had in mind and the `separatorIdx` rename
reads better than my `bang`.
One ordering problem in what landed, though. `LOG` is declared at
`HoodieAvroUtils.java:132`, below `AVRO_VERSION` at `:120`. Static initializers
run in textual order, so `LOG` is still null while `resolveAvroVersion()`
executes in `<clinit>`. If anything in the try block throws (IOException out of
`getResources`/`openStream`, or `IllegalArgumentException` out of
`Properties.load` on a malformed entry), the catch's `LOG.warn(...)` NPEs and
that propagates out of the class initializer as `ExceptionInInitializerError`,
followed by a permanent `NoClassDefFoundError` for the rest of the JVM's life.
That is the exact failure mode I described on the other thread, and it
triggers on the broken-classpath path this PR exists to handle. My suggested
snippet did not say where to put the field, so this one is on me.
Please move the declaration above `AVRO_VERSION`:
```java
public class HoodieAvroUtils {
private static final Logger LOG =
LoggerFactory.getLogger(HoodieAvroUtils.class);
public static final String AVRO_VERSION = resolveAvroVersion();
```
hudi-agent flagged the same thing at line 164, so that comment can be closed
by the same change.
##########
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)) {
+ if (inputStream != null) {
+ Properties avroProperties = new Properties();
+ avroProperties.load(inputStream);
+ String version = avroProperties.getProperty("version");
+ if (version != null) {
+ return version;
+ }
+ }
+ } catch (Exception ignored) {
+ // Ignoring the exception and falling back to the manifest-based version
below
+ }
+ return Schema.class.getPackage().getImplementationVersion();
Review Comment:
The guards on `gteqAvro1_9()` / `gteqAvro1_10()` / `gteqAvro1_12()` are what
I was after, thanks.
The other half of that ask is still open: `resolveAvroVersion()` returns
null silently when both sources miss. Only the exception path logs today, and
the `return Schema.class.getPackage() == null ? null : ...` line at the bottom
hands back null with nothing written anywhere. Combined with the new guards, an
unresolvable version now degrades quietly to pre-1.9 behaviour, which is a
better outcome than the NPE but leaves no trace naming avro or the classpath,
so nobody can diagnose it from a log.
Please warn once at resolution time:
```java
} 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 is on the classpath.", path);
}
return manifestVersion;
```
Note this needs the `LOG` declaration moved above `AVRO_VERSION` to work at
all, see the other thread.
--
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]
