[GitHub] [flink] rkhachatryan commented on a diff in pull request #21405: [FLINK-23035][state/changelog] Add explicit append() to StateChangelogWriter to write metadata

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

rkhachatryan commented on code in PR #21405:
URL: https://github.com/apache/flink/pull/21405#discussion_r1043827297


##########
flink-runtime/src/main/java/org/apache/flink/runtime/state/changelog/StateChange.java:
##########
@@ -26,14 +26,21 @@
 @Internal
 public class StateChange implements Serializable {
 
+    /* For metadata, see FLINK-23035.*/
+    public static final int META_KEY_GROUP = -1;
+
     private static final long serialVersionUID = 1L;
 
     private final int keyGroup;
     private final byte[] change;
 
+    public StateChange(byte[] change) {
+        this.keyGroup = META_KEY_GROUP;
+        this.change = Preconditions.checkNotNull(change);
+    }
+
     public StateChange(int keyGroup, byte[] change) {
-        // todo: enable check in FLINK-23035
-        // Preconditions.checkArgument(keyGroup >= 0);
+        Preconditions.checkArgument(keyGroup >= 0);

Review Comment:
   I think choosing the right constructor is a bit confusing and error-prone now.
   WDYT about making both constructors private and providing clearly named factory methods (e.g. `ofMetadataChange(byte[])` and `ofDataChange(int keyGroup, byte[])`)?



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