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

Wed Jun 20 07:06:49 UTC 2018

Hi Klara,

O  * Spelling error in FlightRecorderServiceV2.java, should be "Flight Recorder" instead of "Flighter Recorder"
Corrected it.

O  * (The class HotSpot23CommercialFeaturesService should probably be called something else, but that could be seen as out of scope for this fix.)
I can open a bug for it.  But why should the name be changed ?  What is the significance of 23 in the filename ?

O  Maybe  JavaVersionSupport.JFR_NOT_COMMERCIAL field should point to a new "JDK_11_EA" instead.

I have changed JDK_11 to JDK_11_EA and JFR_NOT_COMMERCIAL to JDK_11_EA and passed earlyAccess flag as true.  With this the fix works for 11 developer build and EA build.

I again tested 11 and 10 builds with +/-UnlockCommercialFeatures and +/-FlightRecorder flags both open and Oracle jdks.  I also ran the 'verify' tests and they are passing.

Updated Webrev : http://cr.openjdk.java.net/~sballal/JMC-5893/webrev.03/ 

Thanks,

Sharath

From: Klara Ward 
Sent: Tuesday, June 12, 2018 9:26 PM
To: Sharath Ballal
Cc: jmc-dev at openjdk.java.net; Erik Gahlin; Marcus Hirt
Subject: Re: Review Request: JMC-5893: Commercial features check removed in JDK 11

Looks like the version 11-ea is not considered to be greater of equal than 11, which makes this fail right now.
Maybe  JavaVersionSupport.JFR_NOT_COMMERCIAL field should point to a new "JDK_11_EA" instead. This will make the code behave incorrectly for  really early versions of JDK 11, but those are not very interesting anyway.

(Changing the check in JavaVersion.isGreaterOrEqualThan to not includes earlyAccess seems like a more risky, and perhaps incorrect fix.)

I only tested JDK 11 without any flags, 
any other configurations of JDK versions, and +/-UnlockCommercialFeatures and +/-FlightRecorder flags are untested by me.

// Klara

On 2018-06-12 17:34, Klara Ward wrote:

Seems this fix is not done yet (if I did not make a mistake when importing the fix and testing it)

* I imported the patch from webrev.02, but it looks like more changes as needed, since I still got the "Enable Commercial Features" dialog when trying to start a flight recorder for JDK 11, build 17.

* Spelling error in FlightRecorderServiceV2.java, should be "Flight Recorder" instead of "Flighter Recorder"

* (The class HotSpot23CommercialFeaturesService should probably be called something else, but that could be seen as out of scope for this fix.)

// Klara

On 2018-06-06 17:17, Sharath Ballal wrote:

PING.. any more comments.

Thanks,
Sharath

-----Original Message-----
From: Sharath Ballal 
Sent: Monday, June 04, 2018 12:06 PM
To: Erik Gahlin
Cc: HYPERLINK "mailto:jmc-dev at openjdk.java.net"jmc-dev at openjdk.java.net
Subject: RE: Review Request: JMC-5893: Commercial features check removed in JDK 11

Hi Erik,

The UI code is almost similar.  The recent code is as below.  Now the CF is an object stored in FRservice object (and there are two versions of FRservice).

                   IFlightRecorderService flrService = handle.getServiceOrNull(IFlightRecorderService.class);
                   if (flrService == null) {
                          throw new FlightRecorderException(JVMSupportToolkit.getNoFlightRecorderErrorMessage(handle, false));
                   } else if (flrService.isEnabled()
                                  || ControlPanel.askUserForEnable(flrService, Messages.COMMERCIAL_FEATURES_QUESTION)) {
                          MCFile recFile = ControlPanel.getDefaultRecordingFile(recorder.getServerHandle());
                          RecordingWizardModel model = new RecordingWizardModel(flrService, recFile);
                          recorder.resetWarning();
                          return new StartRecordingWizard(model, recorder);
                   } else {
                          return null;
                   }

Our earlier understanding was that the CF flag will be removed from JDK 11 Oracle builds.  Hence I thought that since most of the changes seemed to be in the commercial features, better to contain the changes there and behave as if the commercial flag is enabled for JDK 11 and above (to avoid changes in other places).

Thanks for pointing that CF may be present in JDK 11 Oracle builds though JFR is not a CF.  Now we need a way to differentiate between Oracle JDK 11 with CF option and Oracle JDK 7-10 with CF option.  I have used the java version number to differentiate, but if you feel there is something better, let me know.  

HYPERLINK "http://cr.openjdk.java.net/%7Esballal/JMC-5893/webrev.02/"http://cr.openjdk.java.net/~sballal/JMC-5893/webrev.02/ 

Thanks,
Sharath

-----Original Message-----
From: Erik Gahlin
Sent: Monday, May 28, 2018 4:42 PM
To: Sharath Ballal
Cc: HYPERLINK "mailto:jmc-dev at openjdk.java.net"jmc-dev at openjdk.java.net
Subject: Re: Review Request: JMC-5893: Commercial features check removed in JDK 11

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
HYPERLINK "http://cr.openjdk.java.net/%7Esballal/JMC-5893/webrev.01/"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: HYPERLINK "mailto:jmc-dev at openjdk.java.net"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: HYPERLINK "mailto:jmc-dev at openjdk.java.net"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 HYPERLINK "mailto:sharath.ballal at oracle.com"<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: HYPERLINK "mailto:jmc-dev at openjdk.java.net"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: HYPERLINK "http://cr.openjdk.java.net/%7Esballal/JMC-5893/webrev.00/"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