sijie 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_r298802090
##########
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:
> That is checked before this logic here occurs. Either the user uploaded a
package or the user provided a link, both cases are handled prior to the
classloading logic.
My example is probably not a good one. because the path checking was done
before class loading.
My point is there are many possibilities that exceptions can be thrown
during classloading. 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.
For example, there are many IO operations in a NAR class loader (e.g.
unpacking, listing files and etc). These IO operations can throw exceptions at
any time (e.g. disk corrupted, disk full and many other stuffs). Even you
submitted a good NAR package, but it can still fail. If we don't log the
messages, how can people troubleshoot the problem?
> we can't exactly determine which one of the above 2
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.
I don't really understand why that would be confusing. You can write the
error message 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 ...; Pulsar
attempts to load it as a JAR package but there is an error. Please check the
package and fix the errors."
If you write an error message like above, it is more informative than giving
nothing to the users.
----------------------------------------------------------------
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
