Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
On Aug 18, 2009, at 11:29 PM, Jörg Schaible wrote: Hi Ralph, Ralph Goers wrote at Mittwoch, 19. August 2009 00:54: On Aug 18, 2009, at 12:48 PM, Jörg Schaible wrote: Hi Mario, Mario Ivankovits wrote: I wont say that there aren't other ways to solve that, but using simple inheritance and instanceof is not the correct way. +1 The current solution is somewhat unconventional, but this way you always know exactly which options are available for your FS. That argument doesn't fly. You'd get that from inheritance too. Hmmm ... what I can think of is to refactor things that way: * FileSystemOptions holds just a map of configurations like MapClass, FileSystemOption * FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption options) FileSystemOption then can be a concrete instance of a set of configurations for one specific filesystem, so you might have HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all possible filesystem options. Sure, this completely breaks backward compatibility - and the GlobalFileSystemOptions thing needs to be solved somehow. This already exists with the DefaultFileSystemConfigBuilder that provides the UserAuthenticator. We actually derived from this class which class? DefaultFileSystemConfigBuilder or UserAuthenticator? Some code helps more that a 1000 words: === % public class VFSConfigBuilder extends DefaultFileSystemConfigBuilder { private static final VFSConfigBuilder BUILDER = new VFSConfigBuilder(); public static VFSConfigBuilder getInstance() { return BUILDER; } public void setId(final FileSystemOptions opts, final String id) throws FileSystemException { setParam(opts, id, id); } @Override protected Class? extends FileSystemConfigBuilder getConfigClass() { return VFSConfigBuilder.class; } } === % And despite your comment to Mario, I can overload the static method with a new return type. Welcome to Java 5 :) Maybe we should switch for VFS 2.0 anyway ... Apples and Oranges. The current minimum version is JDK 1.4. Since WebDav is part of VFS it has to abide by that. But yes, if we change the minimum JDK then that can be done. to provide an additional id as system option - simply to control which FileSystem instances are shared (hashCode of the FileSystemOptions is part of this decision). I'm not sure I understand this. Sample code would help. But I would imagine something could be put in place to control which file systems can be combined using the method Mario proposed above. A snippet from the AbstractFileProvider: === % /** * Adds a file system to those cached by this provider. The file system * may implement {...@link VfsComponent}, in which case it is initialised. */ protected void addFileSystem(final Comparable key, final FileSystem fs) throws FileSystemException { // Add to the cache addComponent(fs); FileSystemKey treeKey = new FileSystemKey(key, fs.getFileSystemOptions()); ((AbstractFileSystem) fs).setCacheKey(treeKey); synchronized (this) { fileSystems.put(treeKey, fs); } } /** * Locates a cached file system * * @return The provider, or null if it is not cached. */ protected FileSystem findFileSystem(final Comparable key, final FileSystemOptions fileSystemProps) { FileSystemKey treeKey = new FileSystemKey(key, fileSystemProps); synchronized (this) { return (FileSystem) fileSystems.get(treeKey); } } === % The cache is also triggered by the FileSystemOptions since it is part of the FileSystemKey. However, we use VFS in a JEE environment where we must have some more control over the allocated resources. Currently we cannot close a FileSystem, since it can be used in another thread/session. Unfortunately we're faced now with the effect that over time VFS eats up any connection to our SFTP server and never releases the connections. Since we use VFS to perform some kind of batch process, we could easily release anything related to this batch job right after it is done. Therefore we'd prefer a controlled behaviour for this kind of caching. Yes, I'm familiar with that code. I was actually looking at it because I had concerns that there could be problems with multiple users trying to access the same file at the same time. Because of the code above the issues I was worried about can't happen. With the current design closing a FileSystem in a multithreaded system cannot be done safely. There are race conditions that currently cannot be avoided. SoftRefFilesCache used to call close and I commented it out for that reason. Actually though, closing the FileSystem shouldn't strictly be necessary to manage the connections. For example the Http
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
On 18/08/2009, joe...@apache.org joe...@apache.org wrote: Author: joehni Date: Tue Aug 18 12:22:35 2009 New Revision: 805384 URL: http://svn.apache.org/viewvc?rev=805384view=rev Log: Make FileSystemOptions cloneable (VFS-278). Modified: commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java Modified: commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java URL: http://svn.apache.org/viewvc/commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java?rev=805384r1=805383r2=805384view=diff == --- commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java (original) +++ commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java Tue Aug 18 12:22:35 2009 @@ -29,7 +29,7 @@ * @see org.apache.commons.vfs.provider.sftp.SftpFileSystemConfigBuilder * @see org.apache.commons.vfs.provider.ftp.FtpFileSystemConfigBuilder */ -public class FileSystemOptions +public class FileSystemOptions implements Cloneable { /** The options */ private Map options = new TreeMap(); @@ -161,5 +161,14 @@ // TODO: compare Entry by Entry return 0; } + +/** + * {...@inheritdoc} + */ +public Object clone() { +FileSystemOptions clone = new FileSystemOptions(); +clone.options = new TreeMap(options); +return clone; +} This clone() does not call super.clone() so won't work properly for sub-classes. } - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Hi Sebb, sebb wrote at Dienstag, 18. August 2009 14:48: + +/** + * {...@inheritdoc} + */ +public Object clone() { +FileSystemOptions clone = new FileSystemOptions(); +clone.options = new TreeMap(options); +return clone; +} This clone() does not call super.clone() so won't work properly for sub-classes. I know, but since the key of the map is a private class anyway, it does not make too much sense to derive from FileSystemOptions anyway ... don't you think? - Jörg - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
On 18/08/2009, Jörg Schaible joerg.schai...@gmx.de wrote: Hi Sebb, sebb wrote at Dienstag, 18. August 2009 14:48: + +/** + * {...@inheritdoc} + */ +public Object clone() { +FileSystemOptions clone = new FileSystemOptions(); +clone.options = new TreeMap(options); +return clone; +} This clone() does not call super.clone() so won't work properly for sub-classes. I know, but since the key of the map is a private class anyway, it does not make too much sense to derive from FileSystemOptions anyway ... don't you think? In that case, you could make the class final. In any case, it would be helpful to document that the clone() method is not written to be sub-classable. - Jörg - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
On Tue, Aug 18, 2009 at 7:48 AM, sebb seb...@gmail.com wrote: On 18/08/2009, Jörg Schaible joerg.schai...@gmx.de wrote: Hi Sebb, sebb wrote at Dienstag, 18. August 2009 14:48: + +/** + * {...@inheritdoc} + */ +public Object clone() { +FileSystemOptions clone = new FileSystemOptions(); +clone.options = new TreeMap(options); +return clone; +} This clone() does not call super.clone() so won't work properly for sub-classes. I know, but since the key of the map is a private class anyway, it does not make too much sense to derive from FileSystemOptions anyway ... don't you think? In that case, you could make the class final. In any case, it would be helpful to document that the clone() method is not written to be sub-classable. I actually don't agree that FileSystemOptions shouldn't be subclassed. I have always disliked immensely having to manipulate FileSystemOptions using a ConfigBuilder class. From personal experience, I've found working with it to be awkward and brittle. I would much prefer to have each provider subclass FileSystemOptions and provide the getters and setters there. Then, at least, you could do an instanceof on the FileSystemOptions and determine what options are actually supposed to be there. Ralph
RE: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Hi! From personal experience, I've found working with it to be awkward and brittle. I would much prefer to have each provider subclass FileSystemOptions and provide the getters and setters there. Then, at least, you could do an instanceof on the FileSystemOptions and determine what options are actually supposed to be there. Look, The FileSystemOptions holds options for various filesystem implementations - at the same time! The reason for this is, that, given the url is provided by the user, you simply do not know which real implementation will be used. Thus, you have to provide e.g. ftp settings and sftp settings at the same time and let VFS decide which options to use - depending on the URL provided by the user. Why would you like to pass in an option-set using a concrete config class if the URL is anything else then concrete. Also, a layered filesystem might require different settings e.g. compression-level of a zip-file on an ftp share. This might require you to configure the zip filesystem (compression) AND the ftp filesystem (active/passive mode) Also, the idea was to implement some sort of GlobalFileSystemOptions, the filesystem implementation then does not need to be changed, just the FileSystemOptions have to take care of that. IF you use the options in a way which allows you to know which kind of filesystem will be used - good - but then the builder is just another layer, but not awkward and brittle I think. I wont say that there aren't other ways to solve that, but using simple inheritance and instanceof is not the correct way. Hmmm ... what I can think of is to refactor things that way: * FileSystemOptions holds just a map of configurations like MapClass, FileSystemOption * FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption options) FileSystemOption then can be a concrete instance of a set of configurations for one specific filesystem, so you might have HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all possible filesystem options. Sure, this completely breaks backward compatibility - and the GlobalFileSystemOptions thing needs to be solved somehow. Ciao, Mario - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
sebb wrote: On 18/08/2009, Jörg Schaible joerg.schai...@gmx.de wrote: Hi Sebb, sebb wrote at Dienstag, 18. August 2009 14:48: + +/** + * {...@inheritdoc} + */ +public Object clone() { +FileSystemOptions clone = new FileSystemOptions(); +clone.options = new TreeMap(options); +return clone; +} This clone() does not call super.clone() so won't work properly for sub-classes. I know, but since the key of the map is a private class anyway, it does not make too much sense to derive from FileSystemOptions anyway ... don't you think? In that case, you could make the class final. In any case, it would be helpful to document that the clone() method is not written to be sub-classable. You're right, it should be at least consistent. Committed. - Jörg - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
RE: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
Hi Mario, Mario Ivankovits wrote: Hi! From personal experience, I've found working with it to be awkward and brittle. I would much prefer to have each provider subclass FileSystemOptions and provide the getters and setters there. Then, at least, you could do an instanceof on the FileSystemOptions and determine what options are actually supposed to be there. Look, The FileSystemOptions holds options for various filesystem implementations - at the same time! The reason for this is, that, given the url is provided by the user, you simply do not know which real implementation will be used. Thus, you have to provide e.g. ftp settings and sftp settings at the same time and let VFS decide which options to use - depending on the URL provided by the user. Why would you like to pass in an option-set using a concrete config class if the URL is anything else then concrete. Also, a layered filesystem might require different settings e.g. compression-level of a zip-file on an ftp share. This might require you to configure the zip filesystem (compression) AND the ftp filesystem (active/passive mode) Also, the idea was to implement some sort of GlobalFileSystemOptions, the filesystem implementation then does not need to be changed, just the FileSystemOptions have to take care of that. IF you use the options in a way which allows you to know which kind of filesystem will be used - good - but then the builder is just another layer, but not awkward and brittle I think. I wont say that there aren't other ways to solve that, but using simple inheritance and instanceof is not the correct way. +1 The current solution is somewhat unconventional, but this way you always know exactly which options are available for your FS. Hmmm ... what I can think of is to refactor things that way: * FileSystemOptions holds just a map of configurations like MapClass, FileSystemOption * FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption options) FileSystemOption then can be a concrete instance of a set of configurations for one specific filesystem, so you might have HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all possible filesystem options. Sure, this completely breaks backward compatibility - and the GlobalFileSystemOptions thing needs to be solved somehow. This already exists with the DefaultFileSystemConfigBuilder that provides the UserAuthenticator. We actually derived from this class to provide an additional id as system option - simply to control which FileSystem instances are shared (hashCode of the FileSystemOptions is part of this decision). - Jörg - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: svn commit: r805384 - /commons/proper/vfs/trunk/core/src/main/java/org/apache/commons/vfs/FileSystemOptions.java
On Aug 18, 2009, at 10:18 AM, Mario Ivankovits wrote: Hi! From personal experience, I've found working with it to be awkward and brittle. I would much prefer to have each provider subclass FileSystemOptions and provide the getters and setters there. Then, at least, you could do an instanceof on the FileSystemOptions and determine what options are actually supposed to be there. Look, The FileSystemOptions holds options for various filesystem implementations - at the same time! The reason for this is, that, given the url is provided by the user, you simply do not know which real implementation will be used. Thus, you have to provide e.g. ftp settings and sftp settings at the same time and let VFS decide which options to use - depending on the URL provided by the user. Why would you like to pass in an option-set using a concrete config class if the URL is anything else then concrete. Also, a layered filesystem might require different settings e.g. compression-level of a zip-file on an ftp share. This might require you to configure the zip filesystem (compression) AND the ftp filesystem (active/passive mode) Also, the idea was to implement some sort of GlobalFileSystemOptions, the filesystem implementation then does not need to be changed, just the FileSystemOptions have to take care of that. IF you use the options in a way which allows you to know which kind of filesystem will be used - good - but then the builder is just another layer, but not awkward and brittle I think. No. They are awkward and brittle. WebDavFileSystemConfigBuilder extends HttpFileSystemConfigBuilder. The getInstance method has to return an HttpFileSystemConfigBuilder since you can't change the return value of an overridden method. So code calling getInstance has to then cast it to the correct type to use it. Of course, The name of the method could have been changed but then it would be instantiated differently then every other ConfigBuilder. I wont say that there aren't other ways to solve that, but using simple inheritance and instanceof is not the correct way. Hmmm ... what I can think of is to refactor things that way: * FileSystemOptions holds just a map of configurations like MapClass, FileSystemOption * FileSystemOptions.set(Class vfsFilesystemClass, FileSystemOption options) FileSystemOption then can be a concrete instance of a set of configurations for one specific filesystem, so you might have HttpFileSystemOption, SftpFileSystemOption etc. Each of them holding all possible filesystem options. I actually like this a lot more than the current implementation. However, it wouldn't allow the get and set methods to be exposed at compile time on the root FileSystemOptions class. I think that can be worked around pretty easily. Sure, this completely breaks backward compatibility - and the GlobalFileSystemOptions thing needs to be solved somehow. The ConfigBuilders can be deprecated and I'm pretty sure they can be made to interact with this structure fairly easily. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org