voonhous commented on code in PR #19596:
URL: https://github.com/apache/hudi/pull/19596#discussion_r3774676691
##########
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:
Correction to what I suggested above: rather than moving the field, please
use lombok's `@Slf4j`. It fixes the same ordering problem and is what the rest
of the module does.
We have lombok in the build now (`hudi-common/pom.xml:129`, provided scope),
and `@Slf4j` is the established idiom: 704 files repo-wide, and within
`hudi-common/src/main` it is 106 `@Slf4j` classes against 10 hand-rolled
loggers, none of them recent.
It also removes the hazard by construction rather than by placement. Lombok
injects the generated field at the top of the class body, so it is assigned
first in `<clinit>`. Compiled against lombok 1.18.36 (our pinned version) on
JDK 11:
```
static {};
0: ldc // class OrderProbe
2: invokestatic // LoggerFactory.getLogger
5: putstatic // Field log <- logger assigned first
8: invokestatic // resolveAvroVersion
11: putstatic // Field AVRO_VERSION
```
The hand-written field keeps working only until someone adds another static
field above it; lombok re-derives the position on every compile.
Concretely:
- drop the two `org.slf4j.Logger` / `org.slf4j.LoggerFactory` imports at
lines 67-68
- add `import lombok.extern.slf4j.Slf4j;` at line 41, at the top of the
third-party group -- `lombok` sorts before `org.apache` under the
`org.apache.hudi,*,javax,java,scala` order in `style/checkstyle.xml:294`
- annotate the class with `@Slf4j` and delete the `LOG` field at line 132
- use `log.warn(...)` at line 164 and for the warn-on-null case on the other
thread
The lowercase `log` is fine despite the `ConstantName` check at
`style/checkstyle.xml:307`, since generated fields never appear in the source
that checkstyle parses.
--
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]
