xuzifu666 commented on code in PR #9788:
URL: https://github.com/apache/hudi/pull/9788#discussion_r1338626252
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -48,19 +48,17 @@ public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new HashMap<>();
public static Class<?> getClass(String clazzName) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- synchronized (CLAZZ_CACHE) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- try {
- Class<?> clazz = Class.forName(clazzName);
- CLAZZ_CACHE.put(clazzName, clazz);
- } catch (ClassNotFoundException e) {
- throw new HoodieException("Unable to load class", e);
- }
+ synchronized (CLAZZ_CACHE) {
Review Comment:
> In case of 100 or more threads hitting this block, will we see the same
performance as we have today? if yes, then I am fine with this change. I was
just trying to avoid unnecessary cost when possible as we run a unique case
with lots of Hudi writers/services hitting this block within a single JVM.
make sense，but if the block is a atomic obj like String、Integer this is very
necessay，but map、set and other collection may not fine in doube if way in
synchonize block.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -48,19 +48,17 @@ public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new HashMap<>();
public static Class<?> getClass(String clazzName) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- synchronized (CLAZZ_CACHE) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- try {
- Class<?> clazz = Class.forName(clazzName);
- CLAZZ_CACHE.put(clazzName, clazz);
- } catch (ClassNotFoundException e) {
- throw new HoodieException("Unable to load class", e);
- }
+ synchronized (CLAZZ_CACHE) {
Review Comment:
> In case of 100 or more threads hitting this block, will we see the same
performance as we have today? if yes, then I am fine with this change. I was
just trying to avoid unnecessary cost when possible as we run a unique case
with lots of Hudi writers/services hitting this block within a single JVM.
make sense，but if the block is a atomic obj like String、Integer this is very
necessay，but map、set and other collection may not fine in doube if way in
synchonize block.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -48,19 +48,17 @@ public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new HashMap<>();
public static Class<?> getClass(String clazzName) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- synchronized (CLAZZ_CACHE) {
- if (!CLAZZ_CACHE.containsKey(clazzName)) {
- try {
- Class<?> clazz = Class.forName(clazzName);
- CLAZZ_CACHE.put(clazzName, clazz);
- } catch (ClassNotFoundException e) {
- throw new HoodieException("Unable to load class", e);
- }
+ synchronized (CLAZZ_CACHE) {
Review Comment:
> In case of 100 or more threads hitting this block, will we see the same
performance as we have today? if yes, then I am fine with this change. I was
just trying to avoid unnecessary cost when possible as we run a unique case
with lots of Hudi writers/services hitting this block within a single JVM.
make sense，but if the block is a atomic obj like String、Integer this is very
necessay，but map、set and other collection may not fine in doube if way in
synchonize block.
--
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]
