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

[Langer, Christoph]

Hi Lance,

thanks again. Makes sense to keep comments consistent. So this is what I’m 
going to push tomorrow: http://cr.openjdk.java.net/~clanger/webrevs/8234089.5/

Cheers
Christoph

From: Lance Andersen 
Sent: Mittwoch, 20. November 2019 19:02
Cc: Langer, Christoph ; nio-dev 
; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

Hi Christoph,

Again, thank you for your efforts here.

Overall I think your latest changes look fine.  I would like to suggest that 
for the methods that you added for MR support, that we make sure the 1st 
character in the comment is capitalized prior to your push of the changes.  I 
know this was not that way prior, but I think it helps with consistency.

BTW,  I am not sure that JarFileSystemProvider was actually ever 
used(JarFileSystem was though) as when I look at the installed providers on my 
Mac, I see:

[sun.nio.fs.MacOSXFileSystemProvider@5b2133b1, 
jdk.nio.zipfs.ZipFileSystemProvider@72ea2f77<mailto:jdk.nio.zipfs.ZipFileSystemProvider@72ea2f77>,
 
jdk.internal.jrtfs.JrtFileSystemProvider@33c7353a<mailto:jdk.internal.jrtfs.JrtFileSystemProvider@33c7353a>]


Best
Lance
On Nov 20, 2019, at 10:02 AM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:



On 20/11/2019 13:39, Langer, Christoph wrote:
Hi Alan,

makes sense. I’ve updated the patch: 
http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/

The updated test looks okay.

-Alan

[cid:image001.gif@01D5A0C8.666731C0]<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>

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

[Lance Andersen]

Hi Christoph,

Again, thank you for your efforts here.

Overall I think your latest changes look fine.  I would like to suggest that 
for the methods that you added for MR support, that we make sure the 1st 
character in the comment is capitalized prior to your push of the changes.  I 
know this was not that way prior, but I think it helps with consistency.

BTW,  I am not sure that JarFileSystemProvider was actually ever 
used(JarFileSystem was though) as when I look at the installed providers on my 
Mac, I see:

[sun.nio.fs.MacOSXFileSystemProvider@5b2133b1, 
jdk.nio.zipfs.ZipFileSystemProvider@72ea2f77, 
jdk.internal.jrtfs.JrtFileSystemProvider@33c7353a]


Best
Lance
> On Nov 20, 2019, at 10:02 AM, Alan Bateman  wrote:
> 
> 
> 
> On 20/11/2019 13:39, Langer, Christoph wrote:
>> Hi Alan,
>>  
>> makes sense. I’ve updated the patch: 
>> http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/ 
>> 
> The updated test looks okay.
> 
> -Alan

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Alan Bateman]

On 20/11/2019 13:39, Langer, Christoph wrote:


Hi Alan,

makes sense. I’ve updated the patch: 
http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/ 





The updated test looks okay.

-Alan

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

[Langer, Christoph]

Hi Alan,

makes sense. I’ve updated the patch: 
http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/

Best regards
Christoph

From: Alan Bateman 
Sent: Mittwoch, 20. November 2019 09:33
To: Langer, Christoph ; Lance Andersen 

Cc: nio-dev ; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem


On 20/11/2019 08:15, Langer, Christoph wrote:
Hi Lance,

I’ve taken care of ModulesInCustomFileSystem then, too.

Updated webrev in place…

If the ModulesInCustomFileSystem test really needs to be changed then the 
private method that has been renamed to testZipFileSystem shoud have its 
parameter changed too. The parameter can be renamed to zipfile (or similar) as 
it's a file path to a zip file. While in the area, it should be changed to use 
the 1-arg newFileSystem method, no need to use the system class loader.

-Alan

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

[Alan Bateman]

On 20/11/2019 08:15, Langer, Christoph wrote:


Hi Lance,

I’ve taken care of ModulesInCustomFileSystem then, too.

Updated webrev in place…

If the ModulesInCustomFileSystem test really needs to be changed then 
the private method that has been renamed to testZipFileSystem shoud have 
its parameter changed too. The parameter can be renamed to zipfile (or 
similar) as it's a file path to a zip file. While in the area, it should 
be changed to use the 1-arg newFileSystem method, no need to use the 
system class loader.


-Alan

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

[Langer, Christoph]

Hi Lance,

I’ve taken care of ModulesInCustomFileSystem then, too.

Updated webrev in place…

Cheers
Christoph

From: Lance Andersen 
Sent: Dienstag, 19. November 2019 23:42
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net; nio-dev 
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

Hi Christoph,

Thank you for the follow up.
On Nov 19, 2019, at 5:37 PM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

ModulesInCustomFileSystem

At line 71:
—
/**
 * Test exploded modules in a JAR file system.
 */
———

I will look at the rest of the changes tomorrow

Best
Lance

[cid:image001.gif@01D59F83.08ADFCD0]<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>

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

[Lance Andersen]

Hi Christoph,

Thank you for the follow up.
> On Nov 19, 2019, at 5:37 PM, Langer, Christoph  
> wrote:
> 
> ModulesInCustomFileSystem

At line 71:

—
/**
 * Test exploded modules in a JAR file system.
 */
———

I will look at the rest of the changes tomorrow

Best
Lance

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Langer, Christoph]

Hi Lance,

> 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.

Ok, agreed for MultiReleaseJarTest and JFSTester. I fixed the comments in 
there. In ModulesInCustomFileSystem I can't see comments that explicitly refer 
to JarFileSystem.

> 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.

Ok, I changed it to " initializeReleaseVersion". I didn't follow your 
suggestion because the method not only checks the release version property but 
also triggers some initialization if a version property is found and it's an 
mr-jar.

> 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.
> 

I updated the comment, trying to incorporate your suggestion. How do you like 
it?

> 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
> 

Done.

> 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"

Ok, entryLookup seems the best fit to me. Changed that.

http://cr.openjdk.java.net/~clanger/webrevs/8234089.3/

Thanks again for reviewing.

Cheers
Christoph

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

[Lance Andersen]

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  
> 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  <mailto:lance.ander...@oracle.com>> 
> Sent: Dienstag, 19. November 2019 19:57
> To: Langer, Christoph  <mailto:christoph.lan...@sap.com>>
> Cc: core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>; 
> nio-dev mailto:nio-...@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  <mailto:lance.ander...@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  <mailto:christoph.lan...@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
> 
>  
>  
>  
> <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>
>  
>  <http://oracle.c

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

[Langer, Christoph]

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 
Sent: Dienstag, 19. November 2019 19:57
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net; nio-dev 
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 
mailto:lance.ander...@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 
mailto:christoph.lan...@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/

Cheers
Christoph

Thank you again!

Best
Lance



<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>

[cid:image001.gif@01D59F25.2712D690]<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>

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

[Lance Andersen]

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  
> 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 > > 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/ 
>> 
>> 
>> Cheers
>> Christoph
> 
> Thank you again!
> 
> Best
> Lance
>> 
> 
>  
> 
>   
> 
>  Lance Andersen| 
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Lance Andersen]

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  
> 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/
> 
> Cheers
> Christoph

Thank you again!

Best
Lance
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Langer, Christoph]

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.

> 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.

> 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.

This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/

Cheers
Christoph

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

[Lance Andersen]

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  
> 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/ 
> 
>  
> Best regards
> Christoph
>  
>  
> From: Langer, Christoph 
> Sent: Mittwoch, 13. November 2019 17:42
> To: core-libs-dev@openjdk.java.net ; 
> nio-dev 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 
> 
> Webrev: 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

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Langer, Christoph]

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/

Best regards
Christoph


From: Langer, Christoph
Sent: Mittwoch, 13. November 2019 17:42
To: core-libs-dev@openjdk.java.net; nio-dev 
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
Webrev: 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