yihua commented on code in PR #13772:
URL: https://github.com/apache/hudi/pull/13772#discussion_r2301933414
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -47,10 +47,61 @@ public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new
ConcurrentHashMap<>();
+ private static final String ENABLE_THREAD_CONTEXT_REFLECTION_KEY =
"hoodie.reflection.usethreadcontext";
+ // The properties must be on the classpath for this to evaluate to true.
+ // This allows us to use the thread context class loader in cases
+ // where the jars are dynamically added to the classpath.
+ private static volatile Boolean useThreadContextClassLoader = null;
+
+ /**
+ * Protected method that can be mocked in tests.
+ * This method is called to determine whether to use thread context class
loader.
+ */
+ protected static boolean shouldUseThreadContextClassLoader() {
+ if (useThreadContextClassLoader == null) {
+ synchronized (ReflectionUtils.class) {
+ if (useThreadContextClassLoader == null) {
+ useThreadContextClassLoader = loadConfigValue();
+ }
+ }
+ }
+ return useThreadContextClassLoader;
+ }
+
+ private static boolean loadConfigValue() {
+ // Try to load via reflection with full DFS support
+ try {
+ Class<?> dfsConfigClass = Class.forName(
+ "org.apache.hudi.common.config.DFSPropertiesConfiguration",
Review Comment:
This looks hacky. It would be better to decouple the hadoop `Configuration`
out of `DFSPropertiesConfiguration` so that `DFSPropertiesConfiguration` can be
moved to `hudi-io` module for usage. The goal of `DFSPropertiesConfiguration`
is to load properties from the file and there is no necessity to use hadoop
`Configuration`.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -47,10 +47,61 @@ public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new
ConcurrentHashMap<>();
+ private static final String ENABLE_THREAD_CONTEXT_REFLECTION_KEY =
"hoodie.reflection.usethreadcontext";
+ // The properties must be on the classpath for this to evaluate to true.
+ // This allows us to use the thread context class loader in cases
+ // where the jars are dynamically added to the classpath.
+ private static volatile Boolean useThreadContextClassLoader = null;
+
+ /**
+ * Protected method that can be mocked in tests.
+ * This method is called to determine whether to use thread context class
loader.
+ */
+ protected static boolean shouldUseThreadContextClassLoader() {
+ if (useThreadContextClassLoader == null) {
+ synchronized (ReflectionUtils.class) {
+ if (useThreadContextClassLoader == null) {
+ useThreadContextClassLoader = loadConfigValue();
+ }
+ }
+ }
+ return useThreadContextClassLoader;
+ }
+
+ private static boolean loadConfigValue() {
+ // Try to load via reflection with full DFS support
+ try {
+ Class<?> dfsConfigClass = Class.forName(
+ "org.apache.hudi.common.config.DFSPropertiesConfiguration",
Review Comment:
This looks hacky. It would be better to decouple the hadoop `Configuration`
out of `DFSPropertiesConfiguration` so that `DFSPropertiesConfiguration` can be
moved to `hudi-io` module for usage. The goal of `DFSPropertiesConfiguration`
is to load properties from the file and there is no necessity to use hadoop
`Configuration`.
--
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]
