gaborgsomogyi commented on code in PR #23406:
URL: https://github.com/apache/flink/pull/23406#discussion_r1329728836
##########
flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java:
##########
@@ -68,6 +76,12 @@ public Configuration toConfiguration(CommandLine
commandLine) throws FlinkExcept
return resultingConfiguration;
}
+ private static boolean isHttpsProtocol(URL url) {
+ return url.getProtocol() == null
Review Comment:
Maybe: `return url.getProtocol() != null &&
(url.getProtocol().equalsIgnoreCase("https"));`
##########
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java:
##########
@@ -255,10 +266,22 @@ private RestClusterClient(
RestClusterClientConfiguration.fromConfiguration(configuration);
this.tempDir = tempDir;
+ this.customHttpHeaders =
+ ClientUtils.readHeadersFromEnvironmentVariable(
+ ConfigConstants.FLINK_REST_CLIENT_HEADERS);
+ jobmanagerUrl =
+ new URL(
+
configuration.getBoolean(SecurityOptions.SSL_REST_ENABLED)
Review Comment:
There is already a function to decide this:
`SecurityOptions.isRestSSLEnabled(config)`
##########
flink-core/src/main/java/org/apache/flink/util/NetUtils.java:
##########
@@ -104,7 +104,10 @@ public static InetSocketAddress
parseHostPortAddress(String hostPort) {
*/
private static URL validateHostPortString(String hostPort) {
try {
- URL u = new URL("http://"; + hostPort);
+ URL u =
+ (hostPort.startsWith("http://";) ||
hostPort.startsWith("https://";))
Review Comment:
Some places we use `equalsIgnoreCase` for protocol but here not allowed. It
would be good to make it consistent. Either not allow or allow all the places.
##########
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java:
##########
@@ -255,10 +266,22 @@ private RestClusterClient(
RestClusterClientConfiguration.fromConfiguration(configuration);
this.tempDir = tempDir;
+ this.customHttpHeaders =
+ ClientUtils.readHeadersFromEnvironmentVariable(
+ ConfigConstants.FLINK_REST_CLIENT_HEADERS);
+ jobmanagerUrl =
+ new URL(
+
configuration.getBoolean(SecurityOptions.SSL_REST_ENABLED)
+ ? "https"
+ : "http",
+ configuration.getString(JobManagerOptions.ADDRESS),
+ configuration.getInteger(JobManagerOptions.PORT),
+ configuration.getString(RestOptions.PATH));
+
if (restClient != null) {
this.restClient = restClient;
} else {
- this.restClient = new RestClient(configuration, executorService);
+ this.restClient = RestClient.forUrl(configuration,
executorService, jobmanagerUrl);
Review Comment:
This is never tested with `RestOptions.PATH`. Plz check
`testRESTManualConfigurationOverride`.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
