Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols

[naoto . sato]

Hi Joe,

I revised the changeset, as the cached hash code in DecimalFormatSymbols 
needs to be recalculated when any of the relevant fields is mutated. 
Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8227313/webrev.02/

Naoto

On 1/2/20 2:19 PM, Joe Wang wrote:

Happy New Year, Naoto!

Thanks for the explanation and changes. The changeset looks good to me.

-Joe

On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Happy new year and thanks for your comments. Please see my replies below:

On 12/23/19 5:20 PM, Joe Wang wrote:

Hi Naoto,

Is there a need for an APINote to note the relationship between the 
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator 
methods. The new method did state it "May be different from {@code 
grouping separator} in some locales", but that may be insufficient. 
For example, does setting one method affects the other (seems it 
should since the monetaryGroupingSeparator may be initialized by the 
groupingSeparator as the impl shows)? If yes, how it's affected?


Setting the custom monetary grouping separator will not affect the 
existing normal grouping separator. I added the explanation in the 
method description.




If no, is there a compatibility concern? In the current impl, the new 
get method is used when "isCurrencyFormat" is true while the new set 
method doesn't affect the existing 'groupingSeparator'. For existing 
applications that have a custom grouping separator set through 
setGroupingSeparator, it seems to me the custom separator won't be 
used moving onto the new JDK impl.


Good point. Modified the compatibility risk from minimum to low with 
the explanation.





A minor comment about the definition statement in the following:

 Add the following new serializable field 
in|java.text.DecimalFormatSymbols|class:


|/** * The grouping separator used when formatting currency values. * 
* @serial * @since 15 */ private char monetaryGroupingSeparator;|



and that for the new get method:
 Gets the character used for thousands separator for currencies.


While the meanings are clear, they were not as consistent as that 
between the existing groupingSeparator and its get method, that is:

 Character used for thousands separator.

    and then the get method states:

 Gets the character used for thousands separator.


But this is minor, your call whether to change it or not.


Consistency is important. I replaced all occurrences of "thousands 
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping 
separator(s)."


Here are the modified changeset:

http://cr.openjdk.java.net/~naoto/8227313/webrev.01/

as well as the modified CSR at the same URL.

Naoto



Best,  and have a great Christmas! :-)
Joe

On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8227313

The proposed CSR and changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/

The change introduces the new monetary grouping separator in 
DecimalFormatSymbols, and it is used if a DecimalFormat instance 
designates currency formatting where its pattern includes the 
currency symbol (U+00A5). The change makes use of the CLDR data 
which provides two distinct grouping separators (one for generic, 
the other for currency) in some locales. It also addresses the 
similar cases for the decimal separator.


Naoto

Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols

[Joe Wang]

Hi Naoto,

The change looks fine to me as only monetaryGroupingSeparator was added 
to equals.


I can't help to note though that, all fields participated in the equals 
calculation except exponential. Some of the other fields are in similar 
situations (one is public and the other not), e.g. percent and 
percentText, perMill and perMillText, minusSign and minusSignText, and 
also the currency related fields, but they all are included. It looks 
like exponential was never publicly accessible, but the (1.6) added 
exponentialSeparator became public. It's probably not necessary to 
include all of them in the first place as they are in sync. that is, 
changing one would change the other -- and in this regard, exponential 
is an exception: setExponentialSymbol won't change exponential.


I understand this is all historical and it doesn't affect your 
changeset. If the reason is known, it won't hurt to add some notes as 
the other setXxxText clearly stated the relationship with their non-Text 
representation. If not, it's fine to me to not have to spend the extra time.


Best,
Joe

On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:

Hi Joe,

I revised the changeset, as the cached hash code in 
DecimalFormatSymbols needs to be recalculated when any of the relevant 
fields is mutated. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8227313/webrev.02/

Naoto

On 1/2/20 2:19 PM, Joe Wang wrote:

Happy New Year, Naoto!

Thanks for the explanation and changes. The changeset looks good to me.

-Joe

On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Happy new year and thanks for your comments. Please see my replies 
below:


On 12/23/19 5:20 PM, Joe Wang wrote:

Hi Naoto,

Is there a need for an APINote to note the relationship between the 
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator 
methods. The new method did state it "May be different from {@code 
grouping separator} in some locales", but that may be insufficient. 
For example, does setting one method affects the other (seems it 
should since the monetaryGroupingSeparator may be initialized by 
the groupingSeparator as the impl shows)? If yes, how it's affected?


Setting the custom monetary grouping separator will not affect the 
existing normal grouping separator. I added the explanation in the 
method description.




If no, is there a compatibility concern? In the current impl, the 
new get method is used when "isCurrencyFormat" is true while the 
new set method doesn't affect the existing 'groupingSeparator'. For 
existing applications that have a custom grouping separator set 
through setGroupingSeparator, it seems to me the custom separator 
won't be used moving onto the new JDK impl.


Good point. Modified the compatibility risk from minimum to low with 
the explanation.





A minor comment about the definition statement in the following:

 Add the following new serializable field 
in|java.text.DecimalFormatSymbols|class:


|/** * The grouping separator used when formatting currency values. 
* * @serial * @since 15 */ private char monetaryGroupingSeparator;|



and that for the new get method:
 Gets the character used for thousands separator for 
currencies.



While the meanings are clear, they were not as consistent as that 
between the existing groupingSeparator and its get method, that is:

 Character used for thousands separator.

    and then the get method states:

 Gets the character used for thousands separator.


But this is minor, your call whether to change it or not.


Consistency is important. I replaced all occurrences of "thousands 
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping 
separator(s)."


Here are the modified changeset:

http://cr.openjdk.java.net/~naoto/8227313/webrev.01/

as well as the modified CSR at the same URL.

Naoto



Best,  and have a great Christmas! :-)
Joe

On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8227313

The proposed CSR and changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/

The change introduces the new monetary grouping separator in 
DecimalFormatSymbols, and it is used if a DecimalFormat instance 
designates currency formatting where its pattern includes the 
currency symbol (U+00A5). The change makes use of the CLDR data 
which provides two distinct grouping separators (one for generic, 
the other for currency) in some locales. It also addresses the 
similar cases for the decimal separator.


Naoto

Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy

[Bob Vandette]

Here are a few comments from scanning the webrev.


It looks like you can remove line 151

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java.sdiff.html

151 int[] ints = new int[0];

—

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/src/java.base/share/classes/jdk/internal/platform/Metrics.java.sdiff.html

There are several comments stating that -2 == unlimited.  This is not the case.

@return count of elapsed periods or -2 if the quota is unlimited.

—

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetrics.java.sdiff.html
 


Shouldn’t you just check for cgroupv2 before trying to run the 
testKernelMemoryLimit and testOomKillFlag tests?

—

There are a few places where getPerCpuUsage is returning “new long[0]” but you 
changed the interface to expect null
for not supported.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java.html
 


http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java.html
 


You probably need to check all users of the APIs which used to return array[0] 
which now return null to make sure you
don’t get null pointer exceptions.

One example is testCpuConsumption.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java.html
 


Also, 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java.html:167


You’ll also have to update copyrights to 2020.

Bob.


> On Dec 20, 2019, at 9:50 AM, Severin Gehwolf  wrote:
> 
> Hi Bob,
> 
> Sorry for the delay to get this updated.
> 
> Changes done in this latest webrev:
> 
>   1. Rebased on top of 8226575: OperatingSystemMXBean should be made
>  container aware. File read ops now use privileged code.
>   2. Warning for mixed cgroup controllers and returning null for metrics.
>   3. Added implementations for getBlkIOServiceCount() and
>  getBlkIOServiced() using sum of rios/wios and rbytes/wbytes across
>  devices. Added impl for getTcpMemoryUsage()
>   4. Returning -2 for not supported (if the metric would return long).
>  boolean metrics now return Boolean and null if not supported. Same
>  for array return types. null if not supported for symmetry.
>   5. -XshowSettings:system output has been updated to return "N/A" for
>  when a metric is not available. E.g. "Kernel OOM killer enabled"
>  Boolean.
>   6. Tests have been updated to reflect this.
> 
> webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/
> incremental webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/
> 
> Testing: Docker tests and podman testing on cgroupsv2. I'll run it
> through jdk/submit as well.
> 
> Hopefully this can get pushed with 8230305 early on in the jdk 15 cycle
> :)
> 
> A couple of more inline comments below.
> 
> On Mon, 2019-12-02 at 12:13 -0500, Bob Vandette wrote:
>> Sorry for the delay in responding.  See comments below:
>> 
>>> On Nov 29, 2019, at 4:05 AM, Severin Gehwolf  wrote:
>>> 
>>> On Fri, 2019-11-15 at 17:51 +0100, Severin Gehwolf wrote:
 Hi,
 
 Could I please get reviews of these core-libs, Linux-only, changes to
 the Metrics subsystem? This patch implements cgroupv2 support for
 Metrics which are currently cgroupv1-only. Fedora 31 switched to
 cgroupv2 by default so it's time to get OpenJDK recognize it.
 
 Note that a couple of metrics are no longer supported with cgroupv2.
 Most notably (not an exhaustive list, though):
 
 Metrics.getKernel*() family of methods.
 Metrics.getTcp*() family of methods.
 Metrics.getBlkIO*() family of methods.
 Metrics.isMemoryOOMKillEnabled()
 
 A couple of open questions with regards to that:
 
 1)
 Most API docs of Metrics make no distiction between "unlimited" and
 "not supported", both returning -1 for longs, for example. This is a
 problem, because output of "java -XshowSettings:syst

Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols

[naoto . sato]

Hi Joe,

Thanks again for the review. The reason the way it is is all historical. 
percent/perMill/minusSign all had public APIs for the 'char' version 
since inception, and text version APIs were added later (JDK13). Thus 
they had to be in sync (both fields are accessible through API). On the 
other hand, exponential was private till JDK6, and at that time I guess 
the engineer decided only to expose public access to its text version, 
i.e., effectively deprecate 'char' version interface and field. I guess 
that's why he/she did not bother make them in sync, IMO. So there seems 
to be no explicit reason (to be noted in the source code) for not syncing.


My $.02

Naoto

On 1/3/20 11:40 AM, Joe Wang wrote:

Hi Naoto,

The change looks fine to me as only monetaryGroupingSeparator was added 
to equals.


I can't help to note though that, all fields participated in the equals 
calculation except exponential. Some of the other fields are in similar 
situations (one is public and the other not), e.g. percent and 
percentText, perMill and perMillText, minusSign and minusSignText, and 
also the currency related fields, but they all are included. It looks 
like exponential was never publicly accessible, but the (1.6) added 
exponentialSeparator became public. It's probably not necessary to 
include all of them in the first place as they are in sync. that is, 
changing one would change the other -- and in this regard, exponential 
is an exception: setExponentialSymbol won't change exponential.


I understand this is all historical and it doesn't affect your 
changeset. If the reason is known, it won't hurt to add some notes as 
the other setXxxText clearly stated the relationship with their non-Text 
representation. If not, it's fine to me to not have to spend the extra 
time.


Best,
Joe

On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:

Hi Joe,

I revised the changeset, as the cached hash code in 
DecimalFormatSymbols needs to be recalculated when any of the relevant 
fields is mutated. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8227313/webrev.02/

Naoto

On 1/2/20 2:19 PM, Joe Wang wrote:

Happy New Year, Naoto!

Thanks for the explanation and changes. The changeset looks good to me.

-Joe

On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Happy new year and thanks for your comments. Please see my replies 
below:


On 12/23/19 5:20 PM, Joe Wang wrote:

Hi Naoto,

Is there a need for an APINote to note the relationship between the 
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator 
methods. The new method did state it "May be different from {@code 
grouping separator} in some locales", but that may be insufficient. 
For example, does setting one method affects the other (seems it 
should since the monetaryGroupingSeparator may be initialized by 
the groupingSeparator as the impl shows)? If yes, how it's affected?


Setting the custom monetary grouping separator will not affect the 
existing normal grouping separator. I added the explanation in the 
method description.




If no, is there a compatibility concern? In the current impl, the 
new get method is used when "isCurrencyFormat" is true while the 
new set method doesn't affect the existing 'groupingSeparator'. For 
existing applications that have a custom grouping separator set 
through setGroupingSeparator, it seems to me the custom separator 
won't be used moving onto the new JDK impl.


Good point. Modified the compatibility risk from minimum to low with 
the explanation.





A minor comment about the definition statement in the following:

 Add the following new serializable field 
in|java.text.DecimalFormatSymbols|class:


|/** * The grouping separator used when formatting currency values. 
* * @serial * @since 15 */ private char monetaryGroupingSeparator;|



and that for the new get method:
 Gets the character used for thousands separator for 
currencies.



While the meanings are clear, they were not as consistent as that 
between the existing groupingSeparator and its get method, that is:

 Character used for thousands separator.

    and then the get method states:

 Gets the character used for thousands separator.


But this is minor, your call whether to change it or not.


Consistency is important. I replaced all occurrences of "thousands 
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping 
separator(s)."


Here are the modified changeset:

http://cr.openjdk.java.net/~naoto/8227313/webrev.01/

as well as the modified CSR at the same URL.

Naoto



Best,  and have a great Christmas! :-)
Joe

On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8227313

The proposed CSR and changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/

The change introduces the new monetary grouping separator in 
De

Re: [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols

[Joe Wang]

Hi Naoto,

Historical indeed, and true, it was not exposed prior to JDK6. Just need 
to make sure other classes within the package won't accidentally use its 
set method or mark it if it's 'deprecated'. But it's a minor issue. I'm 
fine with your changeset as is.


Best,
Joe

On 1/3/20 1:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thanks again for the review. The reason the way it is is all 
historical. percent/perMill/minusSign all had public APIs for the 
'char' version since inception, and text version APIs were added later 
(JDK13). Thus they had to be in sync (both fields are accessible 
through API). On the other hand, exponential was private till JDK6, 
and at that time I guess the engineer decided only to expose public 
access to its text version, i.e., effectively deprecate 'char' version 
interface and field. I guess that's why he/she did not bother make 
them in sync, IMO. So there seems to be no explicit reason (to be 
noted in the source code) for not syncing.


My $.02

Naoto

On 1/3/20 11:40 AM, Joe Wang wrote:

Hi Naoto,

The change looks fine to me as only monetaryGroupingSeparator was 
added to equals.


I can't help to note though that, all fields participated in the 
equals calculation except exponential. Some of the other fields are 
in similar situations (one is public and the other not), e.g. percent 
and percentText, perMill and perMillText, minusSign and 
minusSignText, and also the currency related fields, but they all are 
included. It looks like exponential was never publicly accessible, 
but the (1.6) added exponentialSeparator became public. It's probably 
not necessary to include all of them in the first place as they are 
in sync. that is, changing one would change the other -- and in this 
regard, exponential is an exception: setExponentialSymbol won't 
change exponential.


I understand this is all historical and it doesn't affect your 
changeset. If the reason is known, it won't hurt to add some notes as 
the other setXxxText clearly stated the relationship with their 
non-Text representation. If not, it's fine to me to not have to spend 
the extra time.


Best,
Joe

On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:

Hi Joe,

I revised the changeset, as the cached hash code in 
DecimalFormatSymbols needs to be recalculated when any of the 
relevant fields is mutated. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8227313/webrev.02/

Naoto

On 1/2/20 2:19 PM, Joe Wang wrote:

Happy New Year, Naoto!

Thanks for the explanation and changes. The changeset looks good to 
me.


-Joe

On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Happy new year and thanks for your comments. Please see my replies 
below:


On 12/23/19 5:20 PM, Joe Wang wrote:

Hi Naoto,

Is there a need for an APINote to note the relationship between 
the new get/setMonetaryGroupingSeparator and 
get/setGroupingSeparator methods. The new method did state it 
"May be different from {@code grouping separator} in some 
locales", but that may be insufficient. For example, does setting 
one method affects the other (seems it should since the 
monetaryGroupingSeparator may be initialized by the 
groupingSeparator as the impl shows)? If yes, how it's affected?


Setting the custom monetary grouping separator will not affect the 
existing normal grouping separator. I added the explanation in the 
method description.




If no, is there a compatibility concern? In the current impl, the 
new get method is used when "isCurrencyFormat" is true while the 
new set method doesn't affect the existing 'groupingSeparator'. 
For existing applications that have a custom grouping separator 
set through setGroupingSeparator, it seems to me the custom 
separator won't be used moving onto the new JDK impl.


Good point. Modified the compatibility risk from minimum to low 
with the explanation.





A minor comment about the definition statement in the following:

 Add the following new serializable field 
in|java.text.DecimalFormatSymbols|class:


|/** * The grouping separator used when formatting currency 
values. * * @serial * @since 15 */ private char 
monetaryGroupingSeparator;|



and that for the new get method:
 Gets the character used for thousands separator for 
currencies.



While the meanings are clear, they were not as consistent as that 
between the existing groupingSeparator and its get method, that is:

 Character used for thousands separator.

    and then the get method states:

 Gets the character used for thousands separator.


But this is minor, your call whether to change it or not.


Consistency is important. I replaced all occurrences of "thousands 
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping 
separator(s)."


Here are the modified changeset:

http://cr.openjdk.java.net/~naoto/8227313/webrev.01/

as well as the modified CSR at the same URL.

Naoto



Best,  and have a great Christmas! :-)
Joe

On 12/20/19 12:57 PM, naoto.s...@oracle

jpackage and macOS Catalina notarization

[James Elliott]

Hello, everyone, I finally found this list, as well as a way to search it, and 
so hope this question is relevant and appropriate and not already answered.

For some time I have been using an old javapackager along with a newer release 
of jlink to create native macOS installers for a free, open-source Swing 
application, and am very excited to see that JEP-343 is finally on the horizon 
so I soon can stop relying on the ancient javapackager. Still, its ability to 
code sign my installer DMG has been very beneficial to my less-Java-savvy users 
(generally musicians and light/laser/video technicians running stage shows).

Apple’s current operating system, Catalina, adds still more hoops for 
developers to jump through in order to enable their software to be run without 
complaint and complexity: It needs to be notarized (uploaded to Apple and 
scanned for malicious code and other unsafe properties). I am not asking if 
jpackage might assist with the notarization step any time soon; that is 
something that can be accomplished separately after the code-signed package or 
disk image has been produced.

The issue, however, is that for notarization to succeed, the code signing must 
be performed in a manner that causes the application to use the hardened 
runtime, and therefore a set of entitlements must be added in order for Java 
code to run successfully. (These requirements have been temporarily relaxed 
because so few developers were ready for them, but they will be returning 
soon.) I could not see any evidence in the jpackage documentation or help text 
that it could support these code signing options, specifically —timestamp, 
—options runtime, and —entitlements entitlements.plist (for full details on 
getting this process to work, I found the following two articles incredibly 
helpful: http://www.zarkonnen.com/signing_notarizing_catalina 
 and 
http://kothar.net/macos_catalina_java_11 
 ).

Is this something that is on the radar for a future jpackage release? Failing 
that, is there a way to perform the code signing separately and still use 
jpackage to produce the disk image?

Thanks for any thoughts or insight you might be able to share,

-James