dianfu commented on issue #9874: [FLINK-14240][table] Merge table config parameters(TableConfig#getConfiguration) into global job parameters(ExecutionConfig#getGlobalJobParameters) when running with the legacy planner. URL: https://github.com/apache/flink/pull/9874#issuecomment-541005754 @twalthr Thanks a lot for your comments. However, I don't agree with you on some points: > -1 for this PR. When you post your review comments in the PR, it has already indicated that changes are required for this PR. If this is for my previous +1, it's also not necessary. Just as I said "+1 from my side", it only represents that there is no problems from my side. +1 is only for the part which I'm pretty sure, for the other part which I'm not familiar with, I already ping you. That's the reason why I also ping you to review this change as I think you are more familiar with the changes of this PR. > > First of all, it adds tests that don't focus on what should actually be tested. > > Second, it merges the entire table config into global job parameters. This does not fit into the big picture of FLIP-59. Where global job config should just be a single key under the table config and execution config. FLIP-59 is still under discussion and it has not been accepted yet. Although it's a meaningful proposal from my side, it has been inactive for more than one month. Is there any plan to commit it to release 1.10? if not, we should follow the current status. Besides, before FLIP-59 is accepted and committed, any configuration added via TableConfig is global job parameters and so this change is also self-contained and meaningful from this point of view, you can see Blink planner already implemented it in this way. The details could be found [here](https://github.com/apache/flink/blob/d32af521cbe83f88cd0b822c4d752a1b5102c47c/flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/delegation/PlannerBase.scala#L289). Best, Dian
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
