keith-turner commented on code in PR #5726:
URL: https://github.com/apache/accumulo/pull/5726#discussion_r2207995222
##########
core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java:
##########
@@ -66,5 +68,5 @@ default void init(ContextClassLoaderEnvironment env) {}
* consulting this plugin.
* @return the class loader for the given contextName
*/
- ClassLoader getClassLoader(String contextName);
+ ClassLoader getClassLoader(String contextName) throws IOException,
ReflectiveOperationException;
Review Comment:
Is there a benefit to these two specific exceptions? If we want information
to travel through the code via a checked exception, then it may be better to
create a very specific exception related to this SPI. This allows knowing that
class loader creation failed w/o trying to guess at specific reasons/exceptions
that it could fail, the specific reason should be in the cause. In general we
may want to know this type of failure happened, but we probably do not care too
much why it happened. Whenever it happens for any reasons its not good.
```java
// maybe this should extend Exception
/**
* @since 2.1.4
* /
public static class ClassLoaderCreationFailed extends AccumuloException {
public ClassLoaderCreationFailed(Throwable cause) {}
public ClassLoaderCreationFailed(String msg, Throwable cause) {}
}
ClassLoader getClassLoader(String contextName) throws
ClassLoaderCreationFailed;
```
We could also leave this SPI as is and create a new internal exception that
is always thrown when class loading creation fails. This allows this very
specific and important information to travel in the internal code. Could do
the more minimal change below in 2.1 and add the checked exception to the SPI
in 4.0. Not opposed to adding a checked exception in 2.1.4 to the SPI though,
would need to document the breaking change in the release notes.
```java
public class ClassLoaderUtil {
// create this class outside of public API... any code in the class that
attempts to create a classloader and fails should throw this exception
// this could be a checked or runtime exception... not sure which is best
public static class ClassLoaderCreationFailed extends RuntimeException {
}
public static ClassLoader getClassLoader(String context) {
try{
return FACTORY.getClassLoader(context);
} catch (Exception e) {
throw new ClassLoaderCreationFailed("Failed to create context
"+context, e);
}
}
}
```
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
