gemini-code-assist[bot] commented on code in PR #38906:
URL: https://github.com/apache/beam/pull/38906#discussion_r3395554940
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -546,7 +551,9 @@ public TypeConversion<StackManipulation>
createSetterConversions(StackManipulati
return builder
.visit(new
AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(
+ ProtoByteBuddyUtils.class.getClassLoader(),
+ getClassLoadingStrategy(ProtoByteBuddyUtils.class))
Review Comment:
Using `ProtoByteBuddyUtils.class.getClassLoader()` and
`getClassLoadingStrategy(ProtoByteBuddyUtils.class)` can introduce classloader
visibility issues in multi-classloader environments (such as Spark, Flink, or
application servers).
### The Issue
In these environments, the Beam SDK libraries (including
`ProtoByteBuddyUtils`) are often loaded by a parent/system classloader, while
the user's pipeline code and generated protocol buffer classes (e.g.,
`protoClass`) are loaded by a child/user classloader.
By forcing the generated ByteBuddy classes to be loaded into
`ProtoByteBuddyUtils.class.getClassLoader()` (the parent classloader), the
generated classes will not be able to resolve or access the user's proto
classes (which reside in the child classloader). This will result in a
`NoClassDefFoundError` or `ClassNotFoundException` at runtime when the
generated getter/setter/creator is invoked.
### Suggested Approach
To ensure the generated classes can see the user's proto classes, they
should be loaded using the user class's classloader (e.g.,
`protoClass.getClassLoader()`) and a strategy appropriate for that classloader
(e.g., `getClassLoadingStrategy(protoClass)`).
However, if we use `getClassLoadingStrategy(protoClass)`, we must also
ensure that the package injection strategy (`InjectPackageStrategy`) targets
`protoClass`'s package, because lookup-based class loading strategies
(`MethodHandles.privateLookupIn`) require the generated class to be in the same
package as the lookup target.
Please consider if we can use `protoClass` (or `builderClass`) as the target
for both the package injection and the classloading strategy, similar to how it
is handled in other parts of Beam's schema utilities, or if there is an
alternative way to preserve classloader delegation.
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -598,7 +611,9 @@ FieldValueSetter<ProtoBuilderT, Object> createOneOfSetter(
return builder
.visit(new
AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(
+ ProtoByteBuddyUtils.class.getClassLoader(),
+ getClassLoadingStrategy(ProtoByteBuddyUtils.class))
Review Comment:
Similar to the issue described in `createOneOfGetter`, using
`ProtoByteBuddyUtils.class.getClassLoader()` here can cause
`NoClassDefFoundError` in multi-classloader environments where the user's proto
classes are loaded by a child classloader.
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -511,8 +509,15 @@ public TypeConversion<StackManipulation>
createSetterConversions(StackManipulati
int[] keys =
getterMethodMap.keySet().stream().mapToInt(Integer::intValue).toArray();
+ @SuppressWarnings("unchecked")
DynamicType.Builder<FieldValueGetter<@NonNull ProtoT, OneOfType.Value>>
builder =
- ByteBuddyUtils.subclassGetterInterface(BYTE_BUDDY, protoClass,
OneOfType.Value.class);
+ (DynamicType.Builder)
+ BYTE_BUDDY
+ .with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
+ .subclass(
+ TypeDescription.Generic.Builder.parameterizedType(
+ FieldValueGetter.class, protoClass,
OneOfType.Value.class)
+ .build());
Review Comment:
Instead of casting to the raw type `(DynamicType.Builder)`, we can cast
directly to the parameterized type
`(DynamicType.Builder<FieldValueGetter<@NonNull ProtoT, OneOfType.Value>>)` to
avoid raw type usage and make the cast more explicit.
```suggestion
@SuppressWarnings("unchecked")
DynamicType.Builder<FieldValueGetter<@NonNull ProtoT, OneOfType.Value>>
builder =
(DynamicType.Builder<FieldValueGetter<@NonNull ProtoT,
OneOfType.Value>>)
BYTE_BUDDY
.with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
.subclass(
TypeDescription.Generic.Builder.parameterizedType(
FieldValueGetter.class, protoClass,
OneOfType.Value.class)
.build());
```
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -568,9 +575,15 @@ FieldValueSetter<ProtoBuilderT, Object> createOneOfSetter(
boolean contiguous = isContiguous(indices);
int[] keys =
setterMethodMap.keySet().stream().mapToInt(Integer::intValue).toArray();
+ @SuppressWarnings("unchecked")
DynamicType.Builder<FieldValueSetter<ProtoBuilderT, Object>> builder =
- ByteBuddyUtils.subclassSetterInterface(
- BYTE_BUDDY, protoBuilderClass, OneOfType.Value.class);
+ (DynamicType.Builder)
+ BYTE_BUDDY
+ .with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
+ .subclass(
+ TypeDescription.Generic.Builder.parameterizedType(
+ FieldValueSetter.class, protoBuilderClass,
OneOfType.Value.class)
+ .build());
Review Comment:
Instead of casting to the raw type `(DynamicType.Builder)`, we can cast
directly to the parameterized type
`(DynamicType.Builder<FieldValueSetter<ProtoBuilderT, Object>>)` to avoid raw
type usage.
```suggestion
@SuppressWarnings("unchecked")
DynamicType.Builder<FieldValueSetter<ProtoBuilderT, Object>> builder =
(DynamicType.Builder<FieldValueSetter<ProtoBuilderT, Object>>)
BYTE_BUDDY
.with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
.subclass(
TypeDescription.Generic.Builder.parameterizedType(
FieldValueSetter.class, protoBuilderClass,
OneOfType.Value.class)
.build());
```
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -1103,11 +1118,15 @@ static <ProtoBuilderT extends MessageLite.Builder>
SchemaUserTypeCreator createB
List<FieldValueSetter<ProtoBuilderT, Object>> setters,
Schema schema) {
try {
+ @SuppressWarnings("unchecked")
DynamicType.Builder<Supplier<ProtoBuilderT>> builder =
(DynamicType.Builder)
BYTE_BUDDY
- .with(new InjectPackageStrategy(builderClass))
- .subclass(Supplier.class)
+ .with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
+ .subclass(
+ TypeDescription.Generic.Builder.parameterizedType(
+ Supplier.class, builderClass)
+ .build())
Review Comment:
Instead of casting to the raw type `(DynamicType.Builder)`, we can cast
directly to the parameterized type
`(DynamicType.Builder<Supplier<ProtoBuilderT>>)` to avoid raw type usage.
```suggestion
@SuppressWarnings("unchecked")
DynamicType.Builder<Supplier<ProtoBuilderT>> builder =
(DynamicType.Builder<Supplier<ProtoBuilderT>>)
BYTE_BUDDY
.with(new InjectPackageStrategy(ProtoByteBuddyUtils.class))
.subclass(
TypeDescription.Generic.Builder.parameterizedType(
Supplier.class, builderClass)
.build())
```
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -1116,7 +1135,9 @@ static <ProtoBuilderT extends MessageLite.Builder>
SchemaUserTypeCreator createB
new AsmVisitorWrapper.ForDeclaredMethods()
.writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(ReflectHelpers.findClassLoader(),
ClassLoadingStrategy.Default.INJECTION)
+ .load(
+ ProtoByteBuddyUtils.class.getClassLoader(),
+ getClassLoadingStrategy(ProtoByteBuddyUtils.class))
Review Comment:
Similar to the issue described in `createOneOfGetter`, using
`ProtoByteBuddyUtils.class.getClassLoader()` here can cause
`NoClassDefFoundError` in multi-classloader environments where the user's proto
classes are loaded by a child classloader.
--
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]
