RE: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-26 Thread Amit Sapre 
Thanks Alan & mandy for reviews.

Amit

 

From: Alan Bateman 
Sent: Friday, March 23, 2018 9:54 PM
To: Amit Sapre; Mandy Chung; serviceability-dev@openjdk.java.net; 
compiler-...@openjdk.java.net
Subject: Re: RFR : JDK-8071367 - JMX: Remove SNMP support

 

On 23/03/2018 10:43, Amit Sapre wrote:



Thanks all for the inputs.

This webrev addresses the inputs : HYPERLINK 
"http://cr.openjdk.java.net/%7Easapre/webrev/2018/JDK-8071367/webrev.02/"http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.02/

 

I think you need to put {@code ... } around "-Dcom.sun.management.*", otherwise 
looks good to me.

-Alan

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-23 Thread Alan Bateman 

On 23/03/2018 10:43, Amit Sapre wrote:


Thanks all for the inputs.

This webrev addresses the inputs : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.02/ 




I think you need to put {@code ... } around "-Dcom.sun.management.*", 
otherwise looks good to me.


-Alan

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-23 Thread mandy chung 



On 3/23/18 3:43 AM, Amit Sapre wrote:


Thanks all for the inputs.

This webrev addresses the inputs : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.02/ 






Looks good.

Mandy

RE: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-23 Thread Amit Sapre 
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: serviceability-dev@openjdk.java.net; compiler-...@openjdk.java.net
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

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-22 Thread mandy chung 



On 3/22/18 5:36 PM, Martin Buchholz wrote:



On Wed, Mar 7, 2018 at 11:27 PM, Amit Sapre > wrote:



Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367



Is there a reason this bug is not visible?


I don't see any issue opening this JBS issue and so I went ahead and 
change it.


Note that this is an Oracle JDK module and not part of the OpenJDK.

Mandy

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-22 Thread Martin Buchholz 
On Wed, Mar 7, 2018 at 11:27 PM, Amit Sapre  wrote:

>
> Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367
>

Is there a reason this bug is not visible?

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-22 Thread mandy chung 



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 : 
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

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-22 Thread Alan Bateman 



On 22/03/2018 10:03, Amit Sapre wrote:


Thanks Alan and Mandy for inputs.

This webrev : 
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 ...".


-Alan.

RE: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-22 Thread Amit Sapre 
Thanks Alan and Mandy for inputs.

 

This webrev : 
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.

 

Thanks,

Amit

 

From: mandy chung 
Sent: Friday, March 09, 2018 1:33 AM
To: Alan Bateman; Amit Sapre
Cc: serviceability-dev@openjdk.java.net; compiler-...@openjdk.java.net
Subject: Re: RFR : JDK-8071367 - JMX: Remove SNMP support

 

 

On 3/7/18 11:46 PM, Alan Bateman wrote:

On 08/03/2018 07:27, Amit Sapre wrote:



Hello,

 

Please review the changes for removing SNMP support.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Easapre/webrev/2018/JDK-8071367/webrev.00"http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.00

cc'ing compiler-dev for help on javac/resources/legacy.properties. I'm not 100% 
sure if it is used when compiling to old releases or not.


I think legacy.properties is for compiling for older releases prior to 9 and it 
should not be changed.   Let's get the compiler team to confirm.




As you are re-wording the class description for jdk.internal.agent.Agent then 
we might as well get it right. The Agent class loaded and its static no-arg 
startAgent method is invoked when a system property starting with 
"com.sun.management" is specified on the command line. We could expand this to 
include the case where it is started in a running VM too.


Good suggestion.




build.properties - I assume the empty value for excludes shouldn't have a 
continuation character now.

The rest looks good to me.


I look through the webrev.  No other comment besides called out above.

I created https://bugs.openjdk.java.net/browse/JDK-8199358 to track the docs 
update.

Mandy

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-08 Thread mandy chung 



On 3/7/18 11:46 PM, Alan Bateman wrote:

On 08/03/2018 07:27, Amit Sapre wrote:


Hello,

Please review the changes for removing SNMP support.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367

Webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.00 



cc'ing compiler-dev for help on javac/resources/legacy.properties. I'm 
not 100% sure if it is used when compiling to old releases or not.




I think legacy.properties is for compiling for older releases prior to 9 
and it should not be changed.   Let's get the compiler team to confirm.


As you are re-wording the class description for 
jdk.internal.agent.Agent then we might as well get it right. The Agent 
class loaded and its static no-arg startAgent method is invoked when a 
system property starting with "com.sun.management" is specified on the 
command line. We could expand this to include the case where it is 
started in a running VM too.




Good suggestion.

build.properties - I assume the empty value for excludes shouldn't 
have a continuation character now.


The rest looks good to me.



I look through the webrev.  No other comment besides called out above.

I created https://bugs.openjdk.java.net/browse/JDK-8199358 to track the 
docs update.


Mandy

Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-07 Thread Alan Bateman 

On 08/03/2018 07:27, Amit Sapre wrote:


Hello,

Please review the changes for removing SNMP support.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367

Webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.00 



cc'ing compiler-dev for help on javac/resources/legacy.properties. I'm 
not 100% sure if it is used when compiling to old releases or not.


As you are re-wording the class description for jdk.internal.agent.Agent 
then we might as well get it right. The Agent class loaded and its 
static no-arg startAgent method is invoked when a system property 
starting with "com.sun.management" is specified on the command line. We 
could expand this to include the case where it is started in a running 
VM too.


build.properties - I assume the empty value for excludes shouldn't have 
a continuation character now.


The rest looks good to me.

-Alan.