nishita-09 commented on PR #1000: URL: https://github.com/apache/flink-kubernetes-operator/pull/1000#issuecomment-4208728152
> > I have added some notes, overall I think there is too much logic in the configmanager. We should move the getting/accessing of the configs to the Observers, then the FlinkConfigManager should simply always apply when available. > > Sure @gyfora , will make this change. > > This is going to be very critical logic, so the exception handling is critical, it's better to fall back to the previous behaviour (simply use the previous logic if the runtime config cannot be accessed) so that part is good (has to go to the observer too). > > Yes makes sense, even I was of the same opinion. Will take this up. > > Once we restructured this, we should add detailed test coverage for the core logic and error handling :) > > Thanks @gyfora for the review. Will take a look at this and complete this by EOW. @gyfora Please do review the changes, I have moved most of the update runtime config logic to observer where the configurations would be available. Also avoided hardcodings and handled errors as well in case the runtime config is not available/ accessible fallback would be the earlier config. One of my major concerns is that this is very restrictive as we are particularly adding the configs set programmatically to the runtime config. Incase in future there is another config that is added, then explicit effort is required to even update it here. The fundamental constraint is that Flink's REST API exposes the per-job checkpoint config through CheckpointConfigInfo which uses custom JSON field names (mode, interval, timeout...) that don't match Flink config keys (execution.checkpointing.mode, execution.checkpointing.interval, execution.checkpointing.timeout...). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
