klsince commented on code in PR #9681:
URL: https://github.com/apache/pinot/pull/9681#discussion_r1012084995
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -74,9 +84,21 @@ public static void init(PinotConfiguration fsFactoryConfig) {
}
public static PinotFS create(String scheme) {
- PinotFS pinotFS = PINOT_FS_MAP.get(scheme);
- Preconditions.checkState(pinotFS != null, "PinotFS for scheme: %s has not
been initialized", scheme);
- return pinotFS;
+ PinotFSInfo pinotFSInfo = PINOT_FS_MAP.get(scheme);
+ Preconditions.checkState(pinotFSInfo != null, "PinotFS for scheme: %s has
not been initialized", scheme);
+ return pinotFSInfo.getPinotFS();
+ }
+
+ public static PinotFS createNewInstance(String scheme) {
+ PinotFSInfo pinotFSInfo = PINOT_FS_MAP.get(scheme);
+ Preconditions.checkState(pinotFSInfo != null, "PinotFS for scheme: %s has
not been initialized", scheme);
Review Comment:
perhaps make the error msg "FS scheme: %s has not been `registered`" to be
more specific
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -39,24 +39,34 @@ private PinotFSFactory() {
public static final String LOCAL_PINOT_FS_SCHEME = "file";
private static final Logger LOGGER =
LoggerFactory.getLogger(PinotFSFactory.class);
private static final String CLASS = "class";
- private static final Map<String, PinotFS> PINOT_FS_MAP = new HashMap<String,
PinotFS>() {
+ private static final Map<String, PinotFSInfo> PINOT_FS_MAP = new
HashMap<String, PinotFSInfo>() {
{
- put(LOCAL_PINOT_FS_SCHEME, new LocalPinotFS());
+ put(LOCAL_PINOT_FS_SCHEME, new PinotFSInfo(null, null, new
LocalPinotFS()));
}
};
public static void register(String scheme, String fsClassName,
PinotConfiguration fsConfiguration) {
try {
- LOGGER.info("Initializing PinotFS for scheme {}, classname {}", scheme,
fsClassName);
- PinotFS pinotFS = PluginManager.get().createInstance(fsClassName);
- pinotFS.init(fsConfiguration);
- PINOT_FS_MAP.put(scheme, pinotFS);
Review Comment:
The current changes LGTM.
But looks like using a delegate pattern might save more changes (particular
on the caller side),
e.g. call `PINOT_FS_MAP.put(scheme, new NoClosePinotFS(pinotFS))` at L53
here, and add the NoClosePinotFS as a private class in this factory class.
```
private static class NoClosePinotFS implement PinotFS {
...
public NoClosePinotFS(PinotFS delegate) {
_delegate = delegate;
}
// ... delegate all other methods to `_delegate` but leave close() as noop
public void close() {...}
}
// for the close method of PinotFSFactory.java
public void close() {
for (PinotFS pinotFS : PINOT_FS_MAP.values()) {
((NoClosePinotFS)pinotFS)._delegate.close();
}
}
```
cc @Jackie-Jiang guess this is what you proposed as 2nd option?
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/segmentgenerationandpush/SegmentGenerationAndPushTaskUtils.java:
##########
@@ -67,8 +66,7 @@ static PinotFS getOutputPinotFS(Map<String, String>
taskConfigs, URI fileURI)
pinotFS.init(fsProps);
return pinotFS;
Review Comment:
how about add a public method
PinotFSFactory.createNewInstance(fileURIScheme, fsClass, fsProps) or make
PinotFSFactory. createPinotFSInstance() public and call it here?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFSFactory.java:
##########
@@ -74,9 +84,21 @@ public static void init(PinotConfiguration fsFactoryConfig) {
}
public static PinotFS create(String scheme) {
Review Comment:
Looks like renaming this method touches so many places, then perhaps add a
comment for this method that it gets an cached instance and caller shouldn't
close the instance.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
