Thanks all for the inputs. This webrev addresses the inputs : http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.02/
Amit From: mandy chung Sent: Thursday, March 22, 2018 11:15 PM To: Alan Bateman; Amit Sapre Cc: [email protected]; [email protected] Subject: Re: RFR : JDK-8071367 - JMX: Remove SNMP support On 3/22/18 6:12 AM, Alan Bateman wrote: On 22/03/2018 10:03, Amit Sapre wrote: Thanks Alan and Mandy for inputs. This webrev : HYPERLINK "http://cr.openjdk.java.net/%7Easapre/webrev/2018/JDK-8071367/webrev.01/"http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.01/ addresses your comments. I reverted changes for legacy.properties and also rebased the patch for jdk/jdk repo. This looks good, just a few typos in Agent's class description "This class also provide entrypoints for jcmd tool ..." => "This class also provides entry points for the jcmd tool ...". Looks good in general. Since you are cleaning up the comment, some suggestions: Replace line 60-67 with: This class provides the methods to start the management agent. 1. {@link #startAgent} method is invoked by the VM if -Dcom.sun.management.* is set 2. {@link #startLocalManagementAgent} or {@link #startRemoteManagementAgent} is invoked to start the management agent after the VM starts via jcmd ManagementAgent.start and start_local command. line 309-310 can be replaced with /* * Starts the local management agent. * This method is invoked by either startAgent method or * by the VM directly via jcmd ManagementAgent.start_local command. */ line 332-335 can be replaced with /* * This method is invoked by the VM to start the remote management agent * via jcmd ManagementAgent.start command. */ Add the javadoc to startAgent() /* * This method is invoked by the VM to start the management agent * when -Dcom.sun.management.* is set during startup. */ Mandy
