[GitHub] [flink] fsk119 commented on a change in pull request #19070: [FLINK-26618][sql-client] Fix Remove-Jar statement not effective

[GitBox <gi...@apache.org>]

fsk119 commented on a change in pull request #19070:
URL: https://github.com/apache/flink/pull/19070#discussion_r827562586



##########
File path: flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/context/SessionContext.java
##########
@@ -261,8 +261,20 @@ public void addJar(String jarPath) {
             return;
         }
 
+        // merge the jar in config with the jar maintained in session
+        Set<URL> jarsInConfig;
+        try {
+            jarsInConfig =
+                    new HashSet<>(
+                            ConfigUtils.decodeListFromConfig(
+                                    sessionConfiguration, PipelineOptions.JARS, URL::new));
+        } catch (MalformedURLException e) {
+            throw new SqlExecutionException(
+                    "Failed to parse the option `pipeline.jars` in configuration.", e);
+        }

Review comment:
       I think the semantic of the ADD JAR syntax does two things:
   1. ADD the jar URL into the classloader dependencies;
   2. ADD the URL into the config `pipeline.jars` for later upload;
   
   Therefore, I think the REMOVE JAR also needs to do as the ADD JAR does.
   1. REMOVE the jar URL from the classloader dependencies;
   2. REMOVE the URL in the config `pipeline.jars` ;
   
   The main problem is that user may try to SET pipeline.jars in SQL Client. At this point, I think we can't just overwrite the config with the maintained dependencies in the classloader when REMOVE JAR, which causes the user's setting is different with before. I think we should also merge the config and then try to remove. What do you think?




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