[GitHub] [flink] dmvk commented on a change in pull request #17402: [FLINK-24017] Setup Kryo to be usable without flink-scala

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

dmvk commented on a change in pull request #17402:
URL: https://github.com/apache/flink/pull/17402#discussion_r725921039



##########
File path: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java
##########
@@ -86,6 +117,23 @@
         configureKryoLogging();
     }
 
+    @Nullable
+    private static final ChillSerializerRegistrar flinkChillPackageRegistrar =

Review comment:
       nit: uppercase variable name

##########
File path: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java
##########
@@ -45,6 +45,8 @@
 import org.slf4j.Logger;

Review comment:
       OT: There are these legacy fields, starting on L166. Is there anything keeping us from removing them?

##########
File path: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java
##########
@@ -67,6 +69,35 @@
  * <p>This serializer is intended as a fallback serializer for the cases that are not covered by the
  * basic types, tuples, and POJOs.
  *
+ * <p>The set of serializers registered with Kryo via {@link Kryo#register}, with their respective
+ * IDs, depends on whether flink-java or flink-scala are on the classpath. This is for
+ * backwards-compatibility reasons.
+ *
+ * <p>If neither are available (which should only apply to tests in flink-core), then:
+ *
+ * <ul>
+ *   <li>0-9 are used for Java primitives
+ *   <li>10+ are used for user-defined registration
+ * </ul>
+ *
+ * <p>If flink-scala is available, then:
+ *
+ * <ul>
+ *   <li>0-9 are used for Java primitives
+ *   <li>10-72 are used for Scala classes
+ *   <li>73-84 are used for Java classes
+ *   <li>85+ are used for user-defined registration
+ * </ul>

Review comment:
       Should we introduce a safeguard for these boundaries? (we only enforce the lower boundary now)

##########
File path: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java
##########
@@ -67,6 +69,35 @@
  * <p>This serializer is intended as a fallback serializer for the cases that are not covered by the
  * basic types, tuples, and POJOs.
  *
+ * <p>The set of serializers registered with Kryo via {@link Kryo#register}, with their respective
+ * IDs, depends on whether flink-java or flink-scala are on the classpath. This is for
+ * backwards-compatibility reasons.
+ *
+ * <p>If neither are available (which should only apply to tests in flink-core), then:
+ *
+ * <ul>
+ *   <li>0-9 are used for Java primitives
+ *   <li>10+ are used for user-defined registration
+ * </ul>
+ *
+ * <p>If flink-scala is available, then:
+ *
+ * <ul>
+ *   <li>0-9 are used for Java primitives
+ *   <li>10-72 are used for Scala classes
+ *   <li>73-84 are used for Java classes
+ *   <li>85+ are used for user-defined registration
+ * </ul>
+ *
+ * <p>If *only* flink-java is available, then:
+ *
+ * <ul>
+ *   <li>0-9 are used for Java primitives
+ *   <li>10-72 are unused (to maintain compatibility)
+ *   <li>73-84 are used for Java classes
+ *   <li>85+ are used for user-defined registration

Review comment:
       OT: It would be nice to have a free range for registrations we may want to add in the future without breaking the compatibility.
   
   It's just something to keep in mind, as we can not easily introduce this now, but if we ever break the compatibility in the future, it may be worth adding it.




-- 
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: issues-unsubscribe@flink.apache.org

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