Abacn commented on code in PR #36476:
URL: https://github.com/apache/beam/pull/36476#discussion_r2421598998


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/JavaUdfLoader.java:
##########
@@ -125,6 +125,20 @@ public AggregateFn loadAggregateFunction(List<String> 
functionPath, String jarPa
    */
   private File downloadFile(String inputPath, String mimeType) throws 
IOException {
     Preconditions.checkArgument(!inputPath.isEmpty(), "Path cannot be empty.");
+

Review Comment:
   We can leave sql/JavaUdfLoader as is for now:
   
   checked `downloadFile` does not check file existence, and only maintains a 
local cache after file gets downloaded. so pre-staging does not work and not 
actionable here
   
   in general JavaUdfLoader was a pretty bad design such that it needs 
`java.security.PrivilegedAction` to inject arbitrary code to JVM
   
   The java.security.PrivilegedAction interface has been deprecated and 
permanent disabled (became no-op) in JDK 24.
   
   that said JavaUdfLoader for runtime loaded jars is expected to be broken in 
Java 25.



##########
sdks/python/apache_beam/utils/subprocess_server.py:
##########
@@ -431,6 +433,24 @@ def _download_jar_to_cache(
       cached_jar_path (str): The local path where the jar should be cached.
       user_agent (str): The user agent to use when downloading.
     """
+    # Issue warning when downloading from public repositories
+    public_repos = [
+        cls.MAVEN_CENTRAL_REPOSITORY,
+        cls.GOOGLE_MAVEN_MIRROR,
+    ]
+
+    if any(download_url.startswith(repo) for repo in public_repos):
+      _LOGGER.warning(

Review Comment:
   Could logging the destination path (instead of url path) be more helpful in 
terms of instructing user where to stage the jar?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to