RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion

Wed Sep 25 10:58:13 UTC 2019

Hi Lance,

first of all, thanks for doing these updates to zipfs. It gets better and better ��

I’ve been looking at the change now, too. Here are my findings:

JarFileSystem.java:
Line 96: remove
Line 98: space should be between if and (, not after (.
Line 103: there are quite some spaces (too much) between return and o

Generally I’m wondering whether the classes JarFileSystem and JarFileSystemProvider couldn’t be removed at all and we could just add this multi-release implementation part into the ZipFileSystem class. But that could be a subsequent change.

ZipFileSystem.java:
Line 92: private static final String COMPRESSION_METHOD = "compressionMethod";
I’d prefer if we could start the property definition constants with PROP_, so, could you use PROP_COMPRESSION_METHOD? And probably also the constants in line 84-87 should be aligned to PROP_POSIX, PROP_DEFAULT_OWNER … (I was using OPT for “Option” when I created them but probably the word “property” makes more sense here).

Iine 94: public static final String DEFLATED_COMPRESSION_METHOD -> this should probably be private. I’d also rather use COMPRESSION_METHOD_DEFLATED
line 96: public static final String STORED_COMPRESSION_METHOD -> same: should be private and COMPRESSION_METHOD_STORED
line 173: insert a space between use and (STORED or DEFLATED)
line 177: Should be compatibility rather than compatability
method getDefaultCompressionMethod (line 181 ff)

I’d rather code it like:
If (env.containsKey(COMPRESSION_METHOD)) {
    …
} else {
    return isTrue(env, "noCompression") ? METHOD_STORED : METHOD_DEFLATED;
}

Independent of that in lines 184, 186 and 187 you could insert a space after the if. That would match more with the style of the rest of the file. Also line 188, I’d prefer a space between switch and (

Line 202: String.format is missing the argument COMPRESSION_METHOD.

ZipFileSystemProvider.java:
In line 57 and 60, I’d declare the constants rather package private than protected.
Line 59: compatibility
Line 126/127: You can just do “return getZipFileSystem(path, env);”

module-info.java:
line 241: start with a “be” -> will be considered…

MultiReleaseJarTest.java:
Line 132, 133: Add a space after the ,

CompressionModeTest.java:
You could remove the empty lines 153 and 170.

Thanks & Best regards
Christoph

From: nio-dev <nio-dev-bounces at openjdk.java.net> On Behalf Of Lance Andersen
Sent: Dienstag, 24. September 2019 01:42
To: Alan Bateman <Alan.Bateman at oracle.com>
Cc: nio-dev <nio-dev at openjdk.java.net>
Subject: Re: RFR: 8231093: Document the ZIP FS properties noCompression and releaseVersion

Please see  http://cr.openjdk.java.net/~lancea/8231093/webrev.02/index.html

As part of the latest update, I removed the documentation on specifying  a Version Object for the “runtimeVersion” property.

I did however leave the code in place for backward compatibility.   I believe the rest of your input has been addressed.

I also updated the CSR.

Mach5 tiers1-3 run clean

Best
Lance
On Sep 23, 2019, at 4:32 PM, Lance Andersen <lance.andersen at oracle.com<mailto:lance.andersen at oracle.com>> wrote:

Thank you for the feedback Alan.

More below

On Sep 23, 2019, at 2:32 PM, Alan Bateman <Alan.Bateman at oracle.com<mailto:Alan.Bateman at oracle.com>> wrote:

On 22/09/2019 23:43, Lance Andersen wrote:

I updated the patch and the CSR.

The updated webrev is: http://cr.openjdk.java.net/~lancea/8231093/webrev.01/index.html
I went through the spec updates in module-info.java.

"when adding entries ...". Is that also true when someone re-writes a file?
Yes it does.  I can change “adding” to “writing"

Do we need quotes around "STORED" and "DEFLATED"  value to make it clearer that they are strings?
Sure I can add the quotes

"an IllegalArgumentException will be thrown". This could be expanded to say that will be thrown when creating the file system.
I can include the additional wording

For releaseVersion there are 3 list items that start "If the value ..." and one that starts with "If the Object". We should keep the wording consistent. Also it feels like it accepts too many possible value types and maybe some of these should be dropped.

Please see below WRT dropping Version

Could we link "multi-release JAR" to the "Multi-release JAR files" section of the JAR file spec. I think {@docRoot}/../specs/jar/jar.html<mailto:%7b at docRoot%7d/../specs/jar/jar.html> should work.
Sure I can add that…

When the value of releaseVersion is a String or Integer then it's the feature version.  In MR JARs the version is the major version of the Java platform release so "11.0.1" shouldn't be in the example.
I can drop  11.0.1 from the example.  Runtime.Version.parse() will accept “11.0.1” and return “11” which is the only reason I included the example .  I can remove the “11.0.1” from the example.

The paragraph for a value of "runtime" or Runtime.Version is a bit difficult to read. I think you want "runtime" to use Runtime.version().feature(). If a Runtime.Version object is provided then it's feature() method will be invoked. Maybe it would be better to split these out or maybe allowing a Runtime.Version object should be dropped to reduce the number of possible value types.

I am more than happy to drop the Runtime.Version object.  However as you are probably aware it was there previously and while it is probably highly unlikely that anyone is passing a Version object, if we drop it from the new”runtimeVersion” property, I assume we drop it from the “multi-release” property as well?  I can update the CSR to indicate we are dropping Version.

Best
Lance

-Alan

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

[cid:image001.gif at 01D57396.C1EFE230]<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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190925/6ded75d4/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 658 bytes
Desc: image001.gif
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190925/6ded75d4/image001-0001.gif>