jerrypeng commented on a change in pull request #4577: fix issue when 
submitting NAR via file url
URL: https://github.com/apache/pulsar/pull/4577#discussion_r298807111
 
 

 ##########
 File path: 
pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/SinkConfigUtils.java
 ##########
 @@ -318,79 +322,89 @@ public static ExtractedSinkDetails validate(SinkConfig 
sinkConfig, Path archiveP
             throw new IllegalArgumentException("Sink timeout must be a 
positive number");
         }
 
-        String sinkClassName;
-        final Class<?> typeArg;
-        final ClassLoader classLoader;
-        if (!isEmpty(sinkConfig.getClassName())) {
-            sinkClassName = sinkConfig.getClassName();
-            // We really don't know if we should use nar class loader or 
regular classloader
-            ClassLoader jarClassLoader = null;
-            ClassLoader narClassLoader = null;
-            try {
-                jarClassLoader = 
FunctionCommon.extractClassLoader(archivePath, uploadedInputStreamAsFile);
-            } catch (Exception e) {
+
+        Class<?> typeArg;
+        ClassLoader classLoader;
+        String sinkClassName = sinkConfig.getClassName();
+        ClassLoader jarClassLoader = null;
+        ClassLoader narClassLoader = null;
+        try {
+            jarClassLoader = FunctionCommon.extractClassLoader(archivePath, 
sinkPackageFile);
+        } catch (Exception e) {
 
 Review comment:
   > These exceptions can be any IO exceptions or other exception when loading 
the classes. With your code change, these exceptions are dropped and there is 
no useful logging recorded for the users to understand what went wrong.
   
   My change doesn't drop any exceptions and error messages except for the one 
that is not correct
   
   > If we can't determine, why not just giving out the two errors and let 
users to know both doesn't work and what are the errors for each case.
   
   Sure I can add something like "Pulsar can't not determine the package is a 
NAR package or JAR package. Pulsar attempts to load it as a NAR package but 
here is the error ...;

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to