Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 24 May 2024 16:49:25 GMT, Chris Plummer wrote: >> Thanks, appreciate the effort trying to make it perfect. >> Can't quite say "must be null or an array of all null entries" ..because I >> suppose it could be an empty array. >> >> In reality, the only caller is our code that wraps a null Subject value, in >> an array, so it's generally a single null in an array. >> >> I hope we are OK sticking with "which must not contain any non-null entries" >> as that does cover it (and implicitly does tell you an empty array is fine). > > For me the main hold up is using "should". Maybe: > > "Must be null or an array that doesn't contain any non-null entries." OK I think I can get rid of "should" - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613782613
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 24 May 2024 16:43:41 GMT, Kevin Walls wrote: >> How about "must be null or an array of all null entries". You could still >> have an `@apiNote` explaining why. > > Thanks, appreciate the effort trying to make it perfect. > Can't quite say "must be null or an array of all null entries" ..because I > suppose it could be an empty array. > > In reality, the only caller is our code that wraps a null Subject value, in > an array, so it's generally a single null in an array. > > I hope we are OK sticking with "which must not contain any non-null entries" > as that does cover it (and implicitly does tell you an empty array is fine). For me the main hold up is using "should". Maybe: "Must be null or an array that doesn't contain any non-null entries." - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613765813
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 24 May 2024 15:50:00 GMT, Chris Plummer wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
>> line 961:
>>
>>> 959: * @param delegationSubjects should be {@code null}, but a non-null
>>> 960: * array is also accepted for compatibility reasons, which must not
>>> 961: * contain any non-null entries.
>>
>> The wording is bit unusual for a parameter description. Just wondering if
>> might be clearer to say "null or an array of null elements" and put add an
>> `@apiNote` to explain that it allows an array with null elements for
>> compatibility reasons. What you have is okay too course, I'm just trying to
>> think of another way to present this odd case.
>
> How about "must be null or an array of all null entries". You could still
> have an `@apiNote` explaining why.
Thanks, appreciate the effort trying to make it perfect.
Can't quite say "must be null or an array of all null entries" ..because I
suppose it could be an empty array.
In reality, the only caller is our code that wraps a null Subject value, in an
array, so it's generally a single null in an array.
I hope we are OK sticking with "which must not contain any non-null entries" as
that does cover it (and implicitly does tell you an empty array is fine).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613758498
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 17 May 2024 10:35:39 GMT, Alan Bateman wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - add an 'also'
>> - typo
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 961:
>
>> 959: * @param delegationSubjects should be {@code null}, but a non-null
>> 960: * array is also accepted for compatibility reasons, which must not
>> 961: * contain any non-null entries.
>
> The wording is bit unusual for a parameter description. Just wondering if
> might be clearer to say "null or an array of null elements" and put add an
> `@apiNote` to explain that it allows an array with null elements for
> compatibility reasons. What you have is okay too course, I'm just trying to
> think of another way to present this odd case.
How about "must be null or an array of all null entries". You could still have
an `@apiNote` explaining why.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613693492
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 17 May 2024 10:14:43 GMT, Daniel Fuchs wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - add an 'also'
>> - typo
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 979:
>
>> 977: * @throws IOException if a general communication exception
>> occurred.
>> 978: * @throws UnsupportedOperationException if {@code
>> delegationSubjects}
>> 979: * contains any non-null values.
>
> Suggestion:
>
> * @throws UnsupportedOperationException if {@code delegationSubjects}
> * is non-null and contains any non-null values.
Yes, thanks well spotted.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1605235219
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Thu, 16 May 2024 20:17:18 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - add an 'also'
> - typo
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 961:
> 959: * @param delegationSubjects should be {@code null}, but a non-null
> 960: * array is also accepted for compatibility reasons, which must not
> 961: * contain any non-null entries.
The wording is bit unusual for a parameter description. Just wondering if might
be clearer to say "null or an array of null elements" and put add an `@apiNote`
to explain that it allows an array with null elements for compatibility
reasons. What you have is okay too course, I'm just trying to think of another
way to present this odd case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1604740107
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Thu, 16 May 2024 20:17:18 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - add an 'also'
> - typo
Marked as reviewed by dfuchs (Reviewer).
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 979:
> 977: * @throws IOException if a general communication exception occurred.
> 978: * @throws UnsupportedOperationException if {@code
> delegationSubjects}
> 979: * contains any non-null values.
Suggestion:
* @throws UnsupportedOperationException if {@code delegationSubjects}
* is non-null and contains any non-null values.
-
PR Review: https://git.openjdk.org/jdk/pull/19253#pullrequestreview-2062912013
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1604707244
Re: jmx-dev RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
> Running JConsole from a previous JDK, and attaching to jdk-23 (after > [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java > Management Extension (JMX) Subject Delegation feature), the MBean tab is > blank. > > In javax/management/remote/rmi/RMIConnectionImpl.java: > addNotificationListener rejects a non-null delegationSubjects array, but > older JDKs will send such an array. It could accept the array, and only > reject/throw if it contains a non-null Subject (i.e. if an attempt to use > subject delegation is really happening). > > Manually testing JConsole, the MBean tab is fully populated and usable. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - add an 'also' - typo - Changes: - all: https://git.openjdk.org/jdk/pull/19253/files - new: https://git.openjdk.org/jdk/pull/19253/files/6c97b5ed..68c791e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19253=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19253=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19253.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19253/head:pull/19253 PR: https://git.openjdk.org/jdk/pull/19253
