nicoloboschi commented on a change in pull request #14098:
URL: https://github.com/apache/pulsar/pull/14098#discussion_r801806585
##########
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java
##########
@@ -127,6 +127,25 @@
@Slf4j
public class NarClassLoader extends URLClassLoader {
+ public static class Factory {
Review comment:
The important thing is that you don't have to put the starting method on
NarClassLoader (because you can't mock it as well)
So, solutions might be:
1) First level class factory: NarClassLoaderFactory. Same as in this PR, but
as first level which is more readable (`NarClassLoaderFactory.create(...)`)
2) Use a builder pattern NarClassLoaderBuilder and use
(`NarClassLoaderBuilder.newBuilder().withArchive()....build()`) and then mock
the builder in the test
3) Have a inner class inside NarClassLoader responsible for the
NarClassLoader creation. Same as 1) but you won't change the public API. You
still have to make the InternalFactory public in order to be mocked. However,
it's a weird round-trip inside the NarClassLoader. Something like this:
```
class InternalFactory {
NarClassLoader createNarClassLoader(..) {
return ... // createNarClassloader
}
}
public static NarClassLoader getFromArchive(..) {
return InternalFactory.createNarClassLoader(...)
}
```
IMHO all the three options are valid
WDYT?
--
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]
