[GitHub] [flink-connector-kafka] tzulitai commented on a diff in pull request #10: [FLINK-30935][connector/kafka] Add kafka serializers version check when using SimpleVersionedSerializer

["tzulitai (via GitHub)" <gi...@apache.org>]

tzulitai commented on code in PR #10:
URL: https://github.com/apache/flink-connector-kafka/pull/10#discussion_r1152287033


##########
flink-connector-kafka/src/main/java/org/apache/flink/connector/kafka/sink/KafkaCommittableSerializer.java:
##########
@@ -46,6 +46,15 @@ public byte[] serialize(KafkaCommittable state) throws IOException {
 
     @Override
     public KafkaCommittable deserialize(int version, byte[] serialized) throws IOException {
+        switch (version) {
+            case 1:

Review Comment:
   How do we know if there wasn't historical versions where the version number is `0`?
   
   Is there existing migration IT tests that verify, with this change, we can still safely restore from previous versions?
   
   In general, it looks like we're missing some documentation where we historically track what versions have existed before and what their corresponding schema is. I think having a class-level Javadoc to document this is already enough.
   
   @chucheng92 do you think you'd be able to do this as part of this PR contribution? Basically, look at historical changes to confirm that this was indeed the only used version number + document its schema in the class-level Javadoc.



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