RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem

Tue Nov 19 21:16:49 UTC 2019

Hi Christoph,

If you look at MultiReleaseJarTest.java, it explicitly references JAR FS in a comment. Same for JFSTester.java in the @Summary.  These should definitely be updated.  There are comments in ModulesInCustomFileSystem.java as well.

As far as variable names, I think this is nice to clean up but less important as part of this update.

The good news is it is not a big set of changes for the comments so I would address those at a minimum.

Best
Lance

> On Nov 19, 2019, at 4:03 PM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hi Lance,
>  
> I’m actually not so sure about this. While my cleanup change would remove the implementation detail of zipfs to use a class named “JarFileSystem” for multi-release jar peculiarities, I think a user of a FileSystem object upon a jar file is not wrong if he names the variable like jarFileSystem or the like. So I don’t see that such cleanup is really worth the while. Would you be ok with that?
>  
> I’ll get back to you soon with an updated webrev regarding the other changes you suggested.
>  
> Best regards
> Christoph
>  
> From: Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>> 
> Sent: Dienstag, 19. November 2019 19:57
> To: Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> Cc: core-libs-dev at openjdk.java.net <mailto:core-libs-dev at openjdk.java.net>; nio-dev <nio-dev at openjdk.java.net <mailto:nio-dev at openjdk.java.net>>
> Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem
>  
> Hi Christoph,
>  
> Something else that should probably be done as part of this proposed change is  to clean up tests such as NodeCostDumpUtil.java and ModulesInCustomFileSystem.java and a few others as they have methods/fields etc... that make reference to the  Jar File System.  While it does not impact the tests as they are currently written, I think it would make sense to do this for clarity with the change you are making.
>  
> Best
> Lance
> On Nov 18, 2019, at 12:53 PM, Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>> wrote:
>  
> Hi Christoph,
>  
> Sorry for the late reply but I was out of the office Friday
> On Nov 15, 2019, at 3:40 AM, Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>> wrote:
>  
> Hi Lance,
> 
> thanks for reviewing. I've addressed your points:
> 
> 
> 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)
> 
> Done that.
>  
> Thank you.  I do think however we need to change the name of the method to perhaps “checkReleaseVersionProperty” as the current name “initializeMultiReleaseJar”  does not seem quite right to me.
>  
> I would also update the comment for the method to something like:
>  
> ----------------
> Checks to see if the Zip File System property  “releaseVersion” has been specified.  If it has not been specified then check for the existence of the “multi-release” property.   If set and the file represents a multi-release JAR, determine the runtime version to use.
> ----------------
>  
> 
> 
> Could you also add a comment above the isMultiReleaseJar method to for
> clarity going forward (I know there was not one there before)
> 
> Done, too.
>  
> Thank you.
>  
> I think we can tweak the comment slightly to something like
>  
> ———————
> Returns true if the Manifest main attribute “Multi-Release” is set to true; false otherwise
> --------------------
> 
> 
> 
> I might change the name of the lookup field in the method makeParentDirs
> with the addition of the Function lookup on line 126.
> 
> I chose to change the name of the global field in line 126 to be named mrlookup. Also updated the comment.
>  
> I am not sure “mrlookup” is the best name as this field is used with or without a multi-release jar.  Perhaps “IndexNodeNameLookup” or
> “entryLookup"
> 
> 
> This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/ <http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/>
> 
> Cheers
> Christoph
>  
> Thank you again!
>  
> Best
> Lance
> 
>  
>  
> <oracle_sig_logo.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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>  
> <image001.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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>  
> 
> 
>  

 <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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>