[GitHub] [flink] mxm commented on a diff in pull request #23406: [FLINK-32884] [flink-clients] PyFlink remote execution should support URLs with paths and https scheme

["mxm (via GitHub)" <gi...@apache.org>]

mxm commented on code in PR #23406:
URL: https://github.com/apache/flink/pull/23406#discussion_r1324778392


##########
flink-core/src/main/java/org/apache/flink/configuration/RestOptions.java:
##########
@@ -80,6 +80,23 @@ public class RestOptions {
                     .withDescription(
                             "The address that should be used by clients to connect to the server. Attention: This option is respected only if the high-availability configuration is NONE.");
 
+    /** The address that should be used by clients to interact with the server. */
+    @Documentation.Section(Documentation.Sections.COMMON_HOST_PORT)
+    public static final ConfigOption<String> PATH =
+            key("rest.path")
+                    .stringType()
+                    .defaultValue("")
+                    .withDescription(
+                            "The path that should be used by clients to interact to the server which is accessible via URL.");
+
+    /** The address that should be used by clients to interact with the server. */

Review Comment:
   ```suggestion
       /** The protocol that should be used by clients to interact with the server. */
   ```



##########
flink-clients/src/test/java/org/apache/flink/client/cli/DefaultCLITest.java:
##########
@@ -46,6 +46,28 @@ void testCommandLineMaterialization() throws Exception {
 
         assertThat(configuration.get(RestOptions.ADDRESS)).isEqualTo(hostname);
         assertThat(configuration.get(RestOptions.PORT)).isEqualTo(port);
+
+        final String httpProtocol = "http";
+        final String hostnameWithHttpScheme = httpProtocol + "://" + hostname;
+        final String[] httpArgs = {"-m", hostnameWithHttpScheme + ':' + port};
+        final CommandLine httpCommandLine = defaultCLI.parseCommandLineOptions(httpArgs, false);
+
+        Configuration newConfiguration = defaultCLI.toConfiguration(httpCommandLine);
+
+        assertThat(newConfiguration.get(RestOptions.ADDRESS)).isEqualTo(hostname);
+        assertThat(newConfiguration.get(RestOptions.PORT)).isEqualTo(port);
+        assertThat(newConfiguration.get(RestOptions.PROTOCOL)).isEqualTo(httpProtocol);
+
+        final String httpsProtocol = "https";
+        final String hostnameWithHttpsScheme = httpsProtocol + "://" + hostname;
+        final String[] httpsArgs = {"-m", hostnameWithHttpsScheme + ':' + port};
+        final CommandLine httpsCommandLine = defaultCLI.parseCommandLineOptions(httpsArgs, false);
+
+        Configuration httpsConfiguration = defaultCLI.toConfiguration(httpsCommandLine);
+
+        assertThat(httpsConfiguration.get(RestOptions.ADDRESS)).isEqualTo(hostname);
+        assertThat(httpsConfiguration.get(RestOptions.PORT)).isEqualTo(port);
+        assertThat(httpsConfiguration.get(RestOptions.PROTOCOL)).isEqualTo(httpsProtocol);

Review Comment:
   Can we test the added `PATH` here as well?



##########
flink-core/src/main/java/org/apache/flink/configuration/RestOptions.java:
##########
@@ -80,6 +80,23 @@ public class RestOptions {
                     .withDescription(
                             "The address that should be used by clients to connect to the server. Attention: This option is respected only if the high-availability configuration is NONE.");
 
+    /** The address that should be used by clients to interact with the server. */

Review Comment:
   ```suggestion
       /** The path that should be used by clients to interact with the server. */
   ```



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org