flink
flink copied to clipboard
apache
[FLINK-31223][sqlgateway] Introduce getFlinkConfigurationOptions to
Back port of https://github.com/apache/flink/pull/22026 to Flink 1.18.
What is the purpose of the change SqlClient now uses SqlGateway for job submission. It will connect to the gateway's RestServer through the RestClient to interact. when we create the RestClient, we load all the configuration options, but when we create the rest server, we only load the EndpointOptions. As a result, if we want to ensure the security of the connection through SSL, the client will enable SSL, but the server does not. The problem of parameter inconsistency will lead to potential problems in the future. We'd better avoid it directly.
Brief change log Pass all flink parameters to the SqlGatewayRestEndpoint. Verifying this change This change added tests and can be verified by SqlClientSSLTest.
Does this pull request potentially affect one of the following parts: Dependencies (does it add or upgrade a dependency): no The public API, i.e., is any changed class annotated with @Public(Evolving): yes The serializers: no The runtime per-record code paths (performance sensitive): no Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no The S3 file system connector: no Documentation Does this pull request introduce a new feature? no
CI report:
- 54363b3ff123817e11f4ea2eafa694f035162c94 Azure: SUCCESS
Bot commands
The @flinkbot bot supports the following commands:@flinkbot run azurere-run the last Azure build
@reswqa junit errors in the pr build - I will investigate - update it is clean now.
Sorry for the delay. The ci build seems also have some problem, would you mind taking a look? Thanks.
Sorry for the delay. The ci build seems also have some problem, would you mind taking a look? Thanks.
@reswqa no worries - it looks clean now - LGTM . This is something we are looking to get into our build as a priority, it would be fabulous if you could merge this asap. Thank you very much for your support.
I think we should also including 4e6dbe2.
@reswqa Thank you so much for the quick reply. We do not currently use JAVA 17. Can I back port that separately as this is clean and the other fix is associated with a different issue - so the backports are as expected?
We should include it as flink has nightly ci that check jdk17 build also. Otherwise this pr will broke ci pipeline.
We should include it as flink has nightly ci that check jdk17 build also. Otherwise this pr will broke ci pipeline.
@reswqa thanks for the clarification :-)
@reswqa I am seeing errors in the CI junits - like it is not using the -e option in some other tests. Investigating.
@reswqa the CI output shows that the config.yaml is not picked up. This was moved into the base test calls by the fix. On the face of it it looks like the @BeforeEach is not being driven for each test, so the yaml file is not present resulting in the error. Continuing to investigate. The error is: ... Caused by: org.apache.flink.configuration.IllegalConfigurationException: The Flink config file ``` '/tmp/junit1139569232718897600/conf882500211154584393/flink-conf.yaml' (/tmp/junit1139569232718897600/conf882500211154584393/flink-conf.yaml) does not exist. May 21 17:58:07 at org.apache.flink.configuration.GlobalConfiguration.loadConfiguration(GlobalConfiguration.java:141)
@reswqa all clean now. I did not backport the 1.19 change at that method was not there at 1.18. I was hoping that I could backport commit 06b3708 which introduces the getMap change to ReadableConfig , but this is not enough. I am not sure how much I will need to back port to get 118 working; 119 has a Flip that changes configuration substantially. I suggest we leave the fix as is, with the method you added, and not do a large amount of 1.19 -1.18 back ports. WDYT?
I suggest we leave the fix as is, with the method you added, and not do a large amount of 1.19 -1.18 back ports. WDYT?
It looks like we can't avoid a change to the public api, and if so, I suggest we don't include this fix in 1.18. bug-fix release(the next minor release of 1.18) should not make changes to the public API, and that's the rule we follow (the nightly CI also disapproves).