[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #15693: [improve][function] Get function class name from the NAR when using built-in functions

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

nicoloboschi commented on code in PR #15693:
URL: https://github.com/apache/pulsar/pull/15693#discussion_r890925562


##########
pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/functions/FunctionUtils.java:
##########
@@ -98,24 +95,29 @@ public static Functions searchForFunctions(String functionsDirectory, boolean al
         try (DirectoryStream<Path> stream = Files.newDirectoryStream(path, "*.nar")) {
             for (Path archive : stream) {
                 try {
-                    FunctionDefinition cntDef = FunctionUtils.getFunctionDefinition(archive.toString());
+
+                    NarClassLoader ncl = NarClassLoaderBuilder.builder()

Review Comment:
   now everytime `searchForFunctions` is called several `NarClassLoader` are created. I expect to see those to be disposed but I haven't seen the required code changes in the caller methods.
   
   do we really need to bind the NarClassLoader inside FunctionArchive ? IIUC this is done in order to save a ClassLoader creation before running the function, but in this way, we open (and perhaps don't close) different classloaders that aren't needed for the function execution



-- 
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: commits-unsubscribe@pulsar.apache.org

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