[GitHub] [flink] rkhachatryan commented on a diff in pull request #20160: [FLINK-28286] move 'enablechangelog' constant to flink-core module

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

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


##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamGraph.java:
##########
@@ -199,6 +199,7 @@ public StateBackend getStateBackend() {
 
     public void setChangelogStateBackendEnabled(TernaryBoolean changelogStateBackendEnabled) {
         this.changelogStateBackendEnabled = changelogStateBackendEnabled;
+        this.executionConfig.setChangelogStateBackendEnabled(changelogStateBackendEnabled);

Review Comment:
   I think `changelogStateBackendEnabled` field is unnecessary now and its read can be replaced with `executionConfig.isChangelogStateBackendEnabled()`.



##########
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java:
##########
@@ -138,6 +139,8 @@ public class ExecutionConfig implements Serializable, Archiveable<ArchivedExecut
 
     private boolean isLatencyTrackingConfigured = false;
 
+    private TernaryBoolean changelogEnabled = TernaryBoolean.UNDEFINED;
+

Review Comment:
   Have you considered moving the configuration to `JobInformation.jobConfiguration` instead of `ExecutionConfig`?
   
   I think it would be easier for
   - FLINK-26372 because `StateChangelogStorageFactory` requires multiple parameters
   - and probably for FLINK-27692 if there will be more conifg parameters
   
   WDYT?
   
   cc: @zoltar9264



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