gemini-code-assist[bot] commented on code in PR #38906:
URL: https://github.com/apache/beam/pull/38906#discussion_r3391501062
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -598,7 +598,7 @@ FieldValueSetter<ProtoBuilderT, Object> createOneOfSetter(
return builder
.visit(new
AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(ReflectHelpers.findClassLoader(),
getClassLoadingStrategy(protoBuilderClass))
Review Comment:
When using a custom class loading strategy for a specific target class
(`protoBuilderClass`), it is safer and more robust to load the generated class
into the target class's class loader (`protoBuilderClass.getClassLoader()`)
rather than the context class loader (`ReflectHelpers.findClassLoader()`). This
avoids potential class loading issues in environments with complex class loader
hierarchies.
```suggestion
.load(protoBuilderClass.getClassLoader(),
getClassLoadingStrategy(protoBuilderClass))
```
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -1116,7 +1116,7 @@ static <ProtoBuilderT extends MessageLite.Builder>
SchemaUserTypeCreator createB
new AsmVisitorWrapper.ForDeclaredMethods()
.writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(ReflectHelpers.findClassLoader(),
getClassLoadingStrategy(builderClass))
Review Comment:
When using a custom class loading strategy for a specific target class
(`builderClass`), it is safer and more robust to load the generated class into
the target class's class loader (`builderClass.getClassLoader()`) rather than
the context class loader (`ReflectHelpers.findClassLoader()`). This avoids
potential class loading issues in environments with complex class loader
hierarchies.
```suggestion
.load(builderClass.getClassLoader(),
getClassLoadingStrategy(builderClass))
```
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -546,7 +546,7 @@ public TypeConversion<StackManipulation>
createSetterConversions(StackManipulati
return builder
.visit(new
AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(ReflectHelpers.findClassLoader(),
getClassLoadingStrategy(protoClass))
Review Comment:
When using a custom class loading strategy for a specific target class
(`protoClass`), it is safer and more robust to load the generated class into
the target class's class loader (`protoClass.getClassLoader()`) rather than the
context class loader (`ReflectHelpers.findClassLoader()`). This avoids
potential `NoClassDefFoundError` or `ClassCastException` in environments with
complex class loader hierarchies (such as Spark, Flink, or other runner
environments).
```suggestion
.load(protoClass.getClassLoader(),
getClassLoadingStrategy(protoClass))
```
--
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]
