Hi Christoph, Thank you for looking into this.
Overall, I think this is OK. Not sure there is currently any downside to removing the JAR FS right now outside of keeping the multi-release code separate. I would suggesting moving the code added to the constructor for checking the releaseVersion/multi-release properties to a method and grouping it with the other methods added for supporting MR jars around line 1396. (keeps it easier to follow and maintain) Could you also add a comment above the isMultiReleaseJar method to for clarity going forward (I know there was not one there before) I might change the name of the lookup field in the method makeParentDirs with the addition of the Function lookup on line 126. The methods in JarFileSystemProvider getPath and uriToPath are slightly different in ZipFileSystemProvider. That being said, I do not see anything obvious of concern but wanted to point it out. The test changes look fine Again, thanks for your efforts to improve Zip FS Best Lance > On Nov 14, 2019, at 10:23 AM, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > Hi, > > after exchanging some direct mails with Lance, I came to the conclusion that > the current behavior to ignore file system property “multi-release” when > “releaseVersion” is explicitly set to null is the right thing to do. So I > updated the webrev to reinstate this functionality and added explicit > comments as well as augmenting MultiReleaseJarTest.java a little bit to test > that “multi-release” is ignored in the right places. > > This is the new webrev: > http://cr.openjdk.java.net/~clanger/webrevs/8234089.1/ > <http://cr.openjdk.java.net/~clanger/webrevs/8234089.1/> > > Best regards > Christoph > > > From: Langer, Christoph > Sent: Mittwoch, 13. November 2019 17:42 > To: core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>; > nio-dev <nio-...@openjdk.java.net <mailto:nio-...@openjdk.java.net>> > Subject: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and > JarFileSystem > > Hi, > > can I please get reviews for this cleanup change in zipfs. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8234089 > <https://bugs.openjdk.java.net/browse/JDK-8234089> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.0/ > <http://cr.openjdk.java.net/~clanger/webrevs/8234089.0/> > > I figured that JarFileSystemProvider is completely obsolete (please correct > me if I’m wrong) and the reasons for having a class JarFileSystem that > extends ZipFileSystems are very little in my opinion. I think maintainability > is better when the few implementation details of multi release jars live in > ZipFileSystem as well. It saves some code. The only possible drawback is that > ZipFileSystem:: getInode would have an additional call to a lookup function, > that usually is an identity transformation. I would hope however, that > runtime impact is negligible. > > I also fix a small bug when property “releaseVersion” is set to null in the > env map and multi-release contains a value. In the current implementation it > would not consider the “multi-release” value and treat the mr jar as the > current runtime version. I had to do a small fix in MultiReleaseJarTest.java > to make sure the properties list is cleared in every case. > > The jdk/nio/zipfs tests run well after my change. > > Thanks > Christoph <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
