Review Request: JMC-5893: Commercial features check removed in JDK 11

[Erik Gahlin]

Sharath,

Could you name the constant JDK_MANAGEMENT_JFR_MBEAN_NAME instead of JFR2...

It seems incorrect to return true for the method 
isCommercialFeaturesEnabled(), since commercial features is not enabled 
just because the JFR MBean is present.

I haven't looked at the GUI code, but I expect it looks something like this.

ICommercialFeaturesService cf = connection.getService(ICommercialFeaturesService.class);
if (cf != null) {
    // Oracle JDK,
    if (!cf.isCommercialFeaturesEnabled()) {
       Ask user if CF should be unlocked.
       if (user answers yes) {
         cf.enableCommercialFeatures();
       } else {
         return;
       }
    }
}
FlightRecorderService fr = connection.getService(FlightRecorderService.class);
if (fr == null) {
    Inform user the Flight Recorder capabilities are not available
    return;
}
...

If isCommercialFeaturesEnabled() returns true, it will fall through and 
not ask the user. This is why the code works. Problem here seems to be 
that the CF flag may be present in Oracle JDK 11, even though Flight 
Recorder is not a CF. Not sure if it is worth creating a cleaner 
abstraction, rename methods etc, but I will let people who has worked on 
JMC more recently decide on this.

Thanks
Erik

> Erik,
>
> Modified webrev is at http://cr.openjdk.java.net/~sballal/JMC-5893/webrev.01/
>
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Sharath Ballal
> Sent: Saturday, May 26, 2018 11:30 PM
> To: Erik Gahlin
> Cc: jmc-dev at openjdk.java.net
> Subject: RE: Review Request: JMC-5893: Commercial features check removed in JDK 11
>
> Thanks Erik, I will make the changes and send new webrev.
>
>
> Thanks,
> Sharath
>
>
> -----Original Message-----
> From: Erik Gahlin
> Sent: Saturday, May 26, 2018 11:26 PM
> To: Sharath Ballal
> Cc: jmc-dev at openjdk.java.net
> Subject: Re: Review Request: JMC-5893: Commercial features check removed in JDK 11
>
> Hi Sharath,
>
>> On 26 May 2018, at 14:33, Sharath Ballal <sharath.ballal at oracle.com> wrote:
>>
>> Hi Erik,
>>
>> Thanks for the review.
>>
>> I had checked this, but not sure how I missed it.  Now I put some prints and verified that for JDK 9, 10 and 11 the ObjectName for JFR is "jdk.management.jfr:type=FlightRecorder".
>> For 8 & 7 I verified from code that the name is  "com.oracle.jrockit:type=FlightRecorder" but I when I use it I get a InstanceNotFoundException.  Am I missing anything?
> Yes, the MBean needs to be registered in the platform MBeanServer, which can be done using the MissionControlMXBean, which existed in Oracle JDK, but was removed with JDK 9 when the MBean was rewritten into a supported API.
>
>
>> (Actually if I check only for jdk.management.jfr:type=FlightRecorder
>> it is enough since we only want this check to work in 11)
> It should be sufficient.
>
>  From JDK 9, a user can create their own modular runtime image using the jlink tool, so jdk.management.jfr may not be present even in a JDK 11 image.
>
> I think it would be best not to mention the JDK version in the source code, and just focus on if the capability is present or not.
>
> Thanks
> Erik
>   
>
>> javax.management.InstanceNotFoundException: com.oracle.jrockit:type=FlightRecorder
>> 	at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getMBean(DefaultMBeanServerInterceptor.java:1095)
>> 	at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getMBeanInfo(DefaultMBeanServerInterceptor.java:1375)
>> 	...
>> 	...
>>
>> Thanks,
>> Sharath
>>
>>
>> -----Original Message-----
>> From: Erik Gahlin
>> Sent: Friday, May 25, 2018 3:16 PM
>> To: jmc-dev at openjdk.java.net
>> Subject: Re: Review Request: JMC-5893: Commercial features check
>> removed in JDK 11
>>
>> Hi Sharath,
>>
>> As far as I know, the ObjectName for JFR has always been "jdk.management.jfr:type=FlightRecorder" for JDK 9, 10 and 11. For JDK 5, 6, 7 and 8, the name was "com.oracle.jrockit:type=FlightRecorder".
>>
>> If this is incorrect, we have a terrible bug that has gone unnoticed.
>>
>> Erik
>>
>>> Please review the fix for:
>>>
>>>
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JMC-5893
>>>
>>>
>>>
>>> Webrev: http://cr.openjdk.java.net/~sballal/JMC-5893/webrev.00/
>>>
>>>
>>>
>>> Following is the description of the testing done and result:
>>>
>>>
>>>
>>> Ran mvn verify successfully.
>>>
>>> OpenJDK 11 - Does not show "Commercial feature" dialog box.
>>> Recording can be done, but cannot be loaded due to JMC-5895
>>>
>>> OracleJDK 11 - Shows "Commercial feature" dialog box as
>>> "UnlockCommercialFeatures" flag is still present.  Recording can be
>>> done, but cannot be loaded due to JMC-5895
>>>
>>> OpenJDK 10 (10.0.1) - Fails with message "Flight Recorder features are not enabled"
>>>
>>> OracleJDK 10 (10.0.1) - Shows "Commercial feature" dialog box. Recording can be done and gets loaded.
>>>
>>> OpenJDK 9 (9.0.4.1-ojdkbuild) - Fails with message "Flight Recorder features are not enabled"
>>>
>>> OracleJDK 9 (9.0.4)- Shows "Commercial feature" dialog box. Recording can be done and gets loaded.
>>>
>>> OpenJDK 8 (1.8.0_91-3-ojdkbuild) - Fails with message "Flight Recorder features are not enabled"
>>>
>>> OracleJDK 8 (1.8.0_161) - Shows "Commercial feature" dialog box. Recording can be done and gets loaded.
>>>
>>> OpenJDK 7 (1.7.0-u80-unofficial) - Fails with message Flight Recorder
>>> is not supported below 7U4. (same message without fix from a
>>> standalone client)
>>>
>>> OracleJDK 7 (1.7.0_80) - Recording can be done and gets loaded (need to explicitly provide the UnlockCommercialFeatures and FlightRecorder options).
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Sharath
>>>
>>>
>>>
>>>