Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-31223][sqlgateway] Introduce getFlinkConfigurationOptions to #24741

Open
wants to merge 1 commit into
base: release-1.18
Choose a base branch
from

Conversation

davidradl
Copy link

Back port of #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

@davidradl davidradl marked this pull request as ready for review April 29, 2024 11:01
@davidradl
Copy link
Author

@reswqa As discussed please could you merge

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 29, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@davidradl
Copy link
Author

@flinkbot run azure

@davidradl
Copy link
Author

davidradl commented May 1, 2024

@reswqa junit errors in the pr build - I will investigate - update it is clean now.

@davidradl
Copy link
Author

@reswqa are you ok to merge this please?

@reswqa
Copy link
Member

reswqa commented May 14, 2024

Sorry for the delay. The ci build seems also have some problem, would you mind taking a look? Thanks.

@davidradl
Copy link
Author

davidradl commented May 20, 2024

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.

@reswqa
Copy link
Member

reswqa commented May 20, 2024

I think we should also including 4e6dbe2.

@davidradl
Copy link
Author

davidradl commented May 20, 2024

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?

@davidradl
Copy link
Author

@reswqa If you have strong views I can include it in this one.

@reswqa
Copy link
Member

reswqa commented May 20, 2024

We should include it as flink has nightly ci that check jdk17 build also. Otherwise this pr will broke ci pipeline.

@davidradl
Copy link
Author

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

@davidradl
Copy link
Author

@reswqa I have backported the test code as requested.

@davidradl
Copy link
Author

davidradl commented May 21, 2024

@reswqa I have done the same for the 1.19 backport

@davidradl
Copy link
Author

@flinkbot run azure

@davidradl
Copy link
Author

davidradl commented May 21, 2024

@reswqa I am seeing errors in the CI junits - like it is not using the -e option in some other tests. Investigating.

@davidradl
Copy link
Author

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


 
 

@davidradl
Copy link
Author

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

@reswqa
Copy link
Member

reswqa commented May 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants