[GitHub] [flink] becketqin commented on a change in pull request #14303: [FLINK-20379][Connector/Kafka] Improve the KafkaRecordDesrializer interface

[GitBox <gi...@apache.org>]

becketqin commented on a change in pull request #14303:
URL: https://github.com/apache/flink/pull/14303#discussion_r535752205



##########
File path: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/operators/SourceOperator.java
##########
@@ -191,6 +194,11 @@ public void sendSplitRequest() {
 			public void sendSourceEventToCoordinator(SourceEvent event) {
 				operatorEventGateway.sendEventToCoordinator(new SourceEventWrapper(event));
 			}
+
+			@Override
+			public UserCodeClassLoader getUserCodeClassLoader() {
+				return (UserCodeClassLoader) getRuntimeContext().getUserCodeClassLoader();

Review comment:
       This was fixed in a later commit. The change should be put here. I'll rebase the commits.
   
   I did look into `RuntimeContextInitializationContextAdapters`. Please see the other reply for the reason why it was not used here.

##########
File path: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/operators/SourceOperator.java
##########
@@ -197,7 +195,20 @@ public void sendSourceEventToCoordinator(SourceEvent event) {
 
 			@Override
 			public UserCodeClassLoader getUserCodeClassLoader() {
-				return (UserCodeClassLoader) getRuntimeContext().getUserCodeClassLoader();
+				return new UserCodeClassLoader() {

Review comment:
       That was my first attempt as well. But it seems a little ugly for two reasons:
   1. The `RuntimeContextInitializationContextAdapters` is itself a class converting `RuntimeContext` to `InitializationContext`. The scope and usage of that class is quite clear. The `RuntimeContextUserCodeClassLoaderAdapter` is a nested private class. Making only that class public seems a little weird.
   2. The `RuntimeContextInitializationContextAdapters` is affiliated with `DeserializationSchema` which is something that the SourceOperator should not be aware of.
   
   Another 2 options are:
   1. Move the `RuntimeContextUserCodeClassLoaderAdapter` to a separate class.
   2. Add a new method to `RuntimeContext` which returns an actual `UserCodeClassLoader` instead of letting the caller construct by themselves.
   
   Both approach seems a little unnecessary here because the inline class is quite simple.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org