Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi All, After internal discussions, many of the concerns below were addressed and final spec is published at, https://bugs.openjdk.java.net/browse/JDK-8199584 Below is the implementation of the above spec. http://cr.openjdk.java.net/~hb/8187498/webrev.05/ Please review and provide feedback. Thanks Harsha On Wednesday 21 February 2018 08:33 PM, Roger Riggs wrote: Hi, I'm a bit leary of command line arguments being special cased and the corresponding custom mappings to system properties. The convenience is fine but we need to keep the handling out of native code so it is easier to maintain. We don't have a Java API for processing (VM) command line arguments so they are being shoehorned into properties. $.02, Roger On 2/21/2018 12:55 AM, Harsha Wardhana B wrote: On Wednesday 21 February 2018 01:51 AM, mandy chung wrote: The code review and CSR review can be in parallel. For this case, I agree with Kumar to have CSR written that would help the code review. Please specify the behavior and its relationship with jcmd and other relevant diagnosability tools. ok. On 2/20/18 6:41 AM, Kumar Srinivasan wrote: What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. VM options are the last one wins if same option specified multiple times. In this case, it could cause confusion (the last -D option sets the value to 3456 but it's set to a different value). Why not taking the simplest approach - when --start-management-agent is set, it does not accept mixing the old way (i.e. does not accept the management properties to be set via -D)? This RFE is to make the command-line simpler and ease-of-use. I don't see any downside to migrate entirely to the new form. We cannot get rid of specifying options via -D. We have plenty of -D flags but very few have short-hand alternative via --start-management-agent. If management properties are specified by --start-management-agent, the options specified by -D are anyway overwritten if specified. Mandy Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 4/23/18 1:20 PM, Harsha Wardhana B wrote: Hi All, After internal discussions, many of the concerns below were addressed and final spec is published at, https://bugs.openjdk.java.net/browse/JDK-8199584 Below is the implementation of the above spec. http://cr.openjdk.java.net/~hb/8187498/webrev.05/ src/java.base/share/classes/sun/launcher/resources/launcher.properties 112 \ --start-management-agent option=value[:option=value:]\n\ option and value should be and to represent user-supplied name/value. 113 \ start the default management agent with semi-colon separated\n\ 114 \ list of options. Multiple values for an option should be separated\n\ 115 \ by comma. See the java tool guide for a list of options for\n\ 116 \ the default management agent.\n\ typo: "semi-colon separated list" should be colon-separated list "See the java tool guide" - should be "See the Java Monitoring and Management Guide for details" src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java I think the change from single class import to import java.util.* and java.io.* may not be done intentionally (I guess it might be done by IDE). I prefer to keep the orginal single class import. 665 if (args.equals("--start-management-agent") || args.equals("--start-management-agent=")) { 666 // Options are mandatory 667 error(INVALID_OPTION, args); 668 } 709 Optional argStr = vmArguments.stream() 710 .filter(a -> a.startsWith("--start-management-agent")) 711 .reduce((a, b) -> b); The only form passed to the VM is --start-management-agent= since VM option does not take white-space separated argument. I think line 666 and 710 should check for "--start-management-agent=" only. 676 String minusDflag = managementFlags.get(name); minusDflag can be renamed to "propertyName" which is clearer. 714 props.forEach((a, b) -> { 715 String oldVaue = System.getProperty((String) a); 716 if (oldVaue != null && !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options can be set via -D flags or via --start-management-agent but not both"); 718 } 719 System.setProperty((String) a, (String) b); 720 }); 721 } 714 props.forEach((a, b) -> { 715 String oldVaue = System.getProperty((String) a); 716 if (oldVaue != null && !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options can be set via -D flags or via --start-management-agent but not both"); 718 } I think checking if -D is set should be done for each constant defined in ConnectorBootstrap.PropertyNames class instead of each key in props since it could set -Dcom.sun.management.jmxremote.port it didn't set port in --start-management-agent. Do you have such test case covered? 719 System.setProperty((String) a, (String) b);I don't expect --start-management-agent will set the system properties like if -Dcom.sun.management.config.file=config.file which will not set additional system properties, right? It's also not specified in CSR. I see the existing code calling System.getProperty is not modified. I think that may need to be updated too? In addition, as specified in CSR, e.e.g --start-management-agent port=1234:configFile=management.properties:ssl=true:authenticate=false The value specified via the command line takes precedence over the value specified in the config file. port=1234 and ssl=true will take precedence even if those properties are set in management.properties. It seems that this is not covered (or I missed it from the webrev). ConnectorBootstrap.PropertyNames defines the property names. It may be better to extend this class to take the short name and value validator into account (replace the managementMap and validatorMap). You may want to refactor it out as an outer class if needed. I will review the test when the webrev is updated (in case the test will need update). Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On Thursday 26 April 2018 09:09 PM, mandy chung wrote: On 4/23/18 1:20 PM, Harsha Wardhana B wrote: Hi All, After internal discussions, many of the concerns below were addressed and final spec is published at, https://bugs.openjdk.java.net/browse/JDK-8199584 Below is the implementation of the above spec. http://cr.openjdk.java.net/~hb/8187498/webrev.05/ src/java.base/share/classes/sun/launcher/resources/launcher.properties 112 \ --start-management-agent option=value[:option=value:]\n\ option and value should be and to represent user-supplied name/value. The syntax used is borrowed from Java tool guide, https://docs.oracle.com/javase/10/tools/java.htm#JSWOR624 It is also part of java -help output which has been already approved in the CSR. 113 \ start the default management agent with semi-colon separated\n\ 114 \ list of options. Multiple values for an option should be separated\n\ 115 \ by comma. See the java tool guide for a list of options for\n\ 116 \ the default management agent.\n\ typo: "semi-colon separated list" should be colon-separated list "See the java tool guide" - should be "See the Java Monitoring and Management Guide for details" I will fix the typo. But the extended help for java is part of the tool guide and not part of management guide. Hence I would like to keep the original reference to tool guide. src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java I think the change from single class import to import java.util.* and java.io.* may not be done intentionally (I guess it might be done by IDE). I prefer to keep the orginal single class import. ok. 665 if (args.equals("--start-management-agent") || args.equals("--start-management-agent=")) { 666 // Options are mandatory 667 error(INVALID_OPTION, args); 668 } 709 Optional argStr = vmArguments.stream() 710 .filter(a -> a.startsWith("--start-management-agent")) 711 .reduce((a, b) -> b); The only form passed to the VM is --start-management-agent= since VM option does not take white-space separated argument. I think line 666 and 710 should check for "--start-management-agent=" only. ok 676 String minusDflag = managementFlags.get(name); minusDflag can be renamed to "propertyName" which is clearer. ok 714 props.forEach((a, b) -> { 715 String oldVaue = System.getProperty((String) a); 716 if (oldVaue != null && !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options can be set via -D flags or via --start-management-agent but not both"); 718 } 719 System.setProperty((String) a, (String) b); 720 }); 721 } 714 props.forEach((a, b) -> { 715 String oldVaue = System.getProperty((String) a); 716 if (oldVaue != null && !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options can be set via -D flags or via --start-management-agent but not both"); 718 } I think checking if -D is set should be done for each constant defined in ConnectorBootstrap.PropertyNames class instead of each key in props since it could set -Dcom.sun.management.jmxremote.port it didn't set port in --start-management-agent. Do you have such test case covered? Agreed. I will modify the code and add a test case to validate the changes. 719 System.setProperty((String) a, (String) b);I don't expect --start-management-agent will set the system properties like if -Dcom.sun.management.config.file=config.file which will not set additional system properties, right? It's also not specified in CSR. I see the existing code calling System.getProperty is not modified. I think that may need to be updated too? I did not understand. Can you please elaborate. In addition, as specified in CSR, e.e.g --start-management-agent port=1234:configFile=management.properties:ssl=true:authenticate=false The value specified via the command line takes precedence over the value specified in the config file. port=1234 and ssl=true will take precedence even if those properties are set in management.properties. It seems that this is not covered (or I missed it from the webrev). That is a default behavior for all command line flags and hence not part of the webrev. ConnectorBootstrap.PropertyNames defines the property names. It may be better to extend this class to take the short name and value validator into account (replace the managementMap and validatorMap). You may want to refactor it out as an outer class if needed. ConnectorBootstrap.PropertyNames is heavily used in sources as well as tests and hence it is not straight-forward to re factor it. I have used the constants from the latter instead of raw strings for managementMap. I will review the test when the webrev is updated (in case the test will need update). Mandy Thanks Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 4/27/18 3:43 PM, Harsha Wardhana B wrote: On Thursday 26 April 2018 09:09 PM, mandy chung wrote: src/java.base/share/classes/sun/launcher/resources/launcher.properties 112 \ --start-management-agent option=value[:option=value:]\n\ option and value should be and to represent user-supplied name/value. The syntax used is borrowed from Java tool guide, https://docs.oracle.com/javase/10/tools/java.htm#JSWOR624 I checked java -h output. <...> is the format for variables. It is also part of java -help output which has been already approved in the CSR. This is minor editing. Most importantly the output of java -help should be consistent. 113 \ start the default management agent with semi-colon separated\n\ 114 \ list of options. Multiple values for an option should be separated\n\ 115 \ by comma. See the java tool guide for a list of options for\n\ 116 \ the default management agent.\n\ typo: "semi-colon separated list" should be colon-separated list "See the java tool guide" - should be "See the Java Monitoring and Management Guide for details" I will fix the typo. But the extended help for java is part of the tool guide and not part of management guide. Hence I would like to keep the original reference to tool guide. Are you proposing to document all options for management properties in java tool guide? Details about --start-management-agent belongs to the management guide (not java tool guide). There are several options listed in the java -help output belong to other component for -javaagent, -splash, etc. 719 System.setProperty((String) a, (String) b);I don't expect --start-management-agent will set the system properties like if -Dcom.sun.management.config.file=config.file which will not set additional system properties, right? It's also not specified in CSR. I see the existing code calling System.getProperty is not modified. I think that may need to be updated too? I did not understand. Can you please elaborate. When --start-management-agent:port=1234 is set, I don't expect the system property com.sun.management.jmxremote.port will be set but your current implementation does that. I expect it be consistent with -Dcom.sun.management.config.file= is set. What is the current behavior? I'm not sure whether you may run into some issue and hence setting system properties. In addition, as specified in CSR, e.e.g --start-management-agent port=1234:configFile=management.properties:ssl=true:authenticate=false The value specified via the command line takes precedence over the value specified in the config file. port=1234 and ssl=true will take precedence even if those properties are set in management.properties. It seems that this is not covered (or I missed it from the webrev). That is a default behavior for all command line flags and hence not part of the webrev. My comment is specific to "port=1234:configFile=management.properties:ssl=true:authenticate=false" where the value contains port set to 1234, ssl set to true, authenticate set to false explicitly. The issue is about management.properties also specifies the properties for port, ssl, authenticate and the resulting value should be the explicit value specified in command line and then the properties listed in management.properties. The argument parsing of --start-management-agent is done by Agent class. So it should be covered by this change. Please add a test case and you can confirm if it works. ConnectorBootstrap.PropertyNames defines the property names. It may be better to extend this class to take the short name and value validator into account (replace the managementMap and validatorMap). You may want to refactor it out as an outer class if needed. ConnectorBootstrap.PropertyNames is heavily used in sources as well as tests and hence it is not straight-forward to re factor it. I have used the constants from the latter instead of raw strings for managementMap. I only found two tests referring to PropertyNames class: test/jdk/sun/management/jmxremote/bootstrap/RmiBootstrapTest.java and RmiSslNoKeyStoreTest.java I will suggest to add a new class for your change and review. Once we are happy with the new class, then separate the refactoring/renaming in a separate patch and JBS issue. I think that should be straight forward. JDK-8187498 will contain its own change. Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 24/01/2018 11:18, Harsha Wardhana B wrote: Hi, Please review the changes for above enhancement having webrev at, http://cr.openjdk.java.net/~hb/8187498/webrev.00/ One of the guidelines in JEP 293 [1] is to not introduce new options with -X as a prefix. I think a related discussion point is whether it should be a java launcher or VM option. -Alan [1] http://openjdk.java.net/jeps/293
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Harsha, Very nice to see progress on this! Before reviewing, the minimal command line to start up the default management server now becomes -Xmanagement:ssl=false,authenticate=false and if you use a property that doesn't exist, or that is mandatory, you will get an error message stating what is wrong? Could we reduce the command line further, so only a single property is needed: -Xmanagement:secure=false or perhaps: -Xmanagement:unsecure which would set ssl=false,authenticate=false, because that is what you want 99% of the time. Thanks Erik Hi, Please review the changes for above enhancement having webrev at, http://cr.openjdk.java.net/~hb/8187498/webrev.00/ Thanks Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Erik, The minimal command line would be, "-Xmanagement", that will start only the local management server. "-Xmanagement:local=true,port=" will start the remote management server without SSL or authentication. On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote: Hi Harsha, Very nice to see progress on this! Before reviewing, the minimal command line to start up the default management server now becomes -Xmanagement:ssl=false,authenticate=false No. Please refer above for minimal options. and if you use a property that doesn't exist, or that is mandatory, you will get an error message stating what is wrong? If we use property, that doesn't exist, we get invalid option error. As said before, no options are mandatory. /// //./java -Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true HelloWorld// //Error: Invalid option specified: rmiregistry_ssl// /// Could we reduce the command line further, so only a single property is needed: -Xmanagement:secure=false or perhaps: -Xmanagement:unsecure which would set ssl=false,authenticate=false, because that is what you want 99% of the time. Thanks Erik Thanks Harsha Hi, Please review the changes for above enhancement having webrev at, http://cr.openjdk.java.net/~hb/8187498/webrev.00/ Thanks Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Harsha, JEP 293 [1] describes the guidelines for JDK command-line options. As Alan points out, new options should move away from -X prefix but use `--` GNU-style long form option. The guideline says: The use of |-X| as a prefix to indicate "non-standard" options will be discontinued for new options, although command-line help may continue to draw a distinction between more commonly used options and those for advanced use. You can consider `--management` as an alternative. Should this be a launcher option that converts it to the corresponding `-Dcom.sun.management.jmxremote.` rather than a VM option? Mandy [1] http://openjdk.java.net/jeps/293 On 1/24/18 11:21 PM, Harsha Wardhana B wrote: Hi Erik, The minimal command line would be, "-Xmanagement", that will start only the local management server. "-Xmanagement:local=true,port=" will start the remote management server without SSL or authentication. On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote: Hi Harsha, Very nice to see progress on this! Before reviewing, the minimal command line to start up the default management server now becomes -Xmanagement:ssl=false,authenticate=false No. Please refer above for minimal options. and if you use a property that doesn't exist, or that is mandatory, you will get an error message stating what is wrong? If we use property, that doesn't exist, we get invalid option error. As said before, no options are mandatory. /// //./java -Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true HelloWorld// //Error: Invalid option specified: rmiregistry_ssl// /// Could we reduce the command line further, so only a single property is needed: -Xmanagement:secure=false or perhaps: -Xmanagement:unsecure which would set ssl=false,authenticate=false, because that is what you want 99% of the time. Thanks Erik Thanks Harsha Hi, Please review the changes for above enhancement having webrev at, http://cr.openjdk.java.net/~hb/8187498/webrev.00/ Thanks Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. -Harsha On Thursday 25 January 2018 09:31 PM, mandy chung wrote: Hi Harsha, JEP 293 [1] describes the guidelines for JDK command-line options. As Alan points out, new options should move away from -X prefix but use `--` GNU-style long form option. The guideline says: The use of |-X| as a prefix to indicate "non-standard" options will be discontinued for new options, although command-line help may continue to draw a distinction between more commonly used options and those for advanced use. You can consider `--management` as an alternative. Should this be a launcher option that converts it to the corresponding `-Dcom.sun.management.jmxremote.` rather than a VM option? Mandy [1] http://openjdk.java.net/jeps/293 On 1/24/18 11:21 PM, Harsha Wardhana B wrote: Hi Erik, The minimal command line would be, "-Xmanagement", that will start only the local management server. "-Xmanagement:local=true,port=" will start the remote management server without SSL or authentication. On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote: Hi Harsha, Very nice to see progress on this! Before reviewing, the minimal command line to start up the default management server now becomes -Xmanagement:ssl=false,authenticate=false No. Please refer above for minimal options. and if you use a property that doesn't exist, or that is mandatory, you will get an error message stating what is wrong? If we use property, that doesn't exist, we get invalid option error. As said before, no options are mandatory. /// //./java -Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true HelloWorld// //Error: Invalid option specified: rmiregistry_ssl// /// Could we reduce the command line further, so only a single property is needed: -Xmanagement:secure=false or perhaps: -Xmanagement:unsecure which would set ssl=false,authenticate=false, because that is what you want 99% of the time. Thanks Erik Thanks Harsha Hi, Please review the changes for above enhancement having webrev at, http://cr.openjdk.java.net/~hb/8187498/webrev.00/ Thanks Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 29/01/2018 05:20, Harsha Wardhana B wrote: Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. I think Mandy's suggestion is to just transform --management so a form that the VM can read. The launcher will need to replace the space anyway as the VM only accepts "=". -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Alan, I am not fully aware about Java launcher or how it passes options to VM. Let me check with some other folks and get back to you. Thanks Harsha On Monday 29 January 2018 01:55 PM, Alan Bateman wrote: On 29/01/2018 05:20, Harsha Wardhana B wrote: Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. I think Mandy's suggestion is to just transform --management so a form that the VM can read. The launcher will need to replace the space anyway as the VM only accepts "=". -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi All, After internal discussions, below format for management flags was agreed upon. 1. --start-management-agent port=1234,ssl=on (space seperator) or 2. --start-management-agent=port=1234,ssl=on ('=' seperator) If option 1 is specified, it will be converted to option 2 by the java launcher before it is passed onto VM. With above GNU long format for management options, specifying arguments is mandatory unlike before. --start-management-agent will not be recognized in the current format and hence will not default to --start-management-agent=local=true. Below is the webrev with above changes and corresponding tests. http://cr.openjdk.java.net/~hb/8187498/webrev.01/ Please review and comment. Thanks Harsha On Monday 29 January 2018 03:14 PM, Harsha Wardhana B wrote: Hi Alan, I am not fully aware about Java launcher or how it passes options to VM. Let me check with some other folks and get back to you. Thanks Harsha On Monday 29 January 2018 01:55 PM, Alan Bateman wrote: On 29/01/2018 05:20, Harsha Wardhana B wrote: Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. I think Mandy's suggestion is to just transform --management so a form that the VM can read. The launcher will need to replace the space anyway as the VM only accepts "=". -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Harsha, Changes look reasonable to me, couple of things that must be addressed: 1. Since this is a main-stream launcher change with a documented and supported option, a CSR is required, you have to add and document the option in the help page http://hg.openjdk.java.net/jdk/jdk/file/8cc67294ec56/src/java.base/share/classes/sun/launcher/resources/launcher.properties 2. You also have to create a doc bug so that the doc team will document it in the Tools Reference Guide, and link it to this bug. Does it need a Release note ? probably does, in which case you will have to create Release Note subtask and follow the RN process. 3. Is XmanagementAgentTest.java part of tier1 test suite ? If not, then I think it ought to be in tier1 grouping, perhaps best to park this under jdk/tools/launcher/management ? Kumar Hi All, After internal discussions, below format for management flags was agreed upon. 1. --start-management-agent port=1234,ssl=on(space seperator) or 2. --start-management-agent=port=1234,ssl=on('=' seperator) If option 1 is specified, it will be converted to option 2 by the java launcher before it is passed onto VM. With above GNU long format for management options, specifying arguments is mandatory unlike before. --start-management-agent will not be recognized in the current format and hence will not default to --start-management-agent=local=true. Below is the webrev with above changes and corresponding tests. http://cr.openjdk.java.net/~hb/8187498/webrev.01/ Please review and comment. Thanks Harsha On Monday 29 January 2018 03:14 PM, Harsha Wardhana B wrote: Hi Alan, I am not fully aware about Java launcher or how it passes options to VM. Let me check with some other folks and get back to you. Thanks Harsha On Monday 29 January 2018 01:55 PM, Alan Bateman wrote: On 29/01/2018 05:20, Harsha Wardhana B wrote: Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. I think Mandy's suggestion is to just transform --management so a form that the VM can read. The launcher will need to replace the space anyway as the VM only accepts "=". -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi Kumar, Thanks for your inputs. On Wednesday 07 February 2018 09:02 PM, Kumar Srinivasan wrote: Hi Harsha, Changes look reasonable to me, couple of things that must be addressed: 1. Since this is a main-stream launcher change with a documented and supported option, a CSR is required, you have to add and document the option in the help page http://hg.openjdk.java.net/jdk/jdk/file/8cc67294ec56/src/java.base/share/classes/sun/launcher/resources/launcher.properties 2. You also have to create a doc bug so that the doc team will document it in the Tools Reference Guide, and link it to this bug. Does it need a Release note ? probably does, in which case you will have to create Release Note subtask and follow the RN process. Will take care of above items. 3. Is XmanagementAgentTest.java part of tier1 test suite ? If not, then I think it ought to be in tier1 grouping, perhaps best to park this under jdk/tools/launcher/management ? Ok. I will move the test in subsequent webrev. -Harsha Kumar Hi All, After internal discussions, below format for management flags was agreed upon. 1. --start-management-agent port=1234,ssl=on (space seperator) or 2. --start-management-agent=port=1234,ssl=on ('=' seperator) If option 1 is specified, it will be converted to option 2 by the java launcher before it is passed onto VM. With above GNU long format for management options, specifying arguments is mandatory unlike before. --start-management-agent will not be recognized in the current format and hence will not default to --start-management-agent=local=true. Below is the webrev with above changes and corresponding tests. http://cr.openjdk.java.net/~hb/8187498/webrev.01/ Please review and comment. Thanks Harsha On Monday 29 January 2018 03:14 PM, Harsha Wardhana B wrote: Hi Alan, I am not fully aware about Java launcher or how it passes options to VM. Let me check with some other folks and get back to you. Thanks Harsha On Monday 29 January 2018 01:55 PM, Alan Bateman wrote: On 29/01/2018 05:20, Harsha Wardhana B wrote: Hi Mandy,Alan, Thanks for your inputs. If I keep it as launcher option, it may need to know JMX agent flags which may need to be extended in future. I would prefer making it a VM option. I will make the required changes and send out an updated webrev. I think Mandy's suggestion is to just transform --management so a form that the VM can read. The launcher will need to replace the space anyway as the VM only accepts "=". -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 07/02/2018 09:19, Harsha Wardhana B wrote: Hi All, After internal discussions, below format for management flags was agreed upon. 1. --start-management-agent port=1234,ssl=on (space seperator) or 2. --start-management-agent=port=1234,ssl=on ('=' seperator) If option 1 is specified, it will be converted to option 2 by the java launcher before it is passed onto VM. With above GNU long format for management options, specifying arguments is mandatory unlike before. --start-management-agent will not be recognized in the current format and hence will not default to --start-management-agent=local=true. `--start-management-agent ` make sense and it wouldn't be hard to introduce `--start-local-management-agent` if really needed. One point that needs discussion is whether the needs something, maybe an agent name or kind, to make it clear what agent should be started. Today we have a JMX management agent, Oracle JDK downloads additionally have a SNMP management agent. There are several 3rd party exporters. If AgentProvider were promoted to an exported package then it would make this pluggibility a lot cleaner and would mean the --start-management-agent could specify the agent name/kind without needing hacks or forwarding by the JMX agent. -Alan
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 2/7/18 1:19 AM, Harsha Wardhana B wrote: --start-management-agent will not be recognized in the current format and hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Below is the webrev with above changes and corresponding tests. http://cr.openjdk.java.net/~hb/8187498/webrev.01/ arguments.cpp 3216 if (FLAG_SET_CMDLINE(bool, ManagementServer, true) != Flag::SUCCESS) { 3217 return JNI_EINVAL; 3218 } 3219 // management agent in module jdk.management.agent 3220 if (!create_numbered_property("jdk.module.addmods", "jdk.management.agent", addmods_count++)) { 3221 return JNI_ENOMEM; 3222 } - you can consider refactor this to replace this block and line 2988-3994. 3224 jio_fprintf(defaultStream::output_stream(), 3225 "-Xmanagement is not supported in this VM.\n"); -Xmanagement not yet renamed java.c To set the precedence for future VM long form options, I suggest to rename IsManagementOption to IsVMLongFormOption and change the error message not specific to management agent. Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. parseXmgmtArgs should be renamed as no longer -Xmgmt. Better not to tightly coupled with the option name. 291 if(args.startsWith("--start-management-agent")) { 292 return parseXmgmtArgs(args); 293 } What is this change for? Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi, Please find below the revised webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.02/ On Friday 09 February 2018 05:07 AM, mandy chung wrote: On 2/7/18 1:19 AM, Harsha Wardhana B wrote: > > --start-management-agent will not be recognized in the current format and > hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Yes. It requires lot more changes to launcher. Hence optional argument to --start-management-agent was not added in order to keep the launcher impact minimum. > Below is the webrev with above changes and corresponding tests. > > http://cr.openjdk.java.net/~hb/8187498/webrev.01/ arguments.cpp 3216 if (FLAG_SET_CMDLINE(bool, ManagementServer, true) != Flag::SUCCESS) { 3217 return JNI_EINVAL; 3218 } 3219 // management agent in module jdk.management.agent 3220 if (!create_numbered_property("jdk.module.addmods", "jdk.management.agent", addmods_count++)) { 3221 return JNI_ENOMEM; 3222 } - you can consider refactor this to replace this block and line 2988-3994. 3224 jio_fprintf(defaultStream::output_stream(), 3225 "-Xmanagement is not supported in this VM.\n"); -Xmanagement not yet renamed java.c To set the precedence for future VM long form options, I suggest to rename IsManagementOption to IsVMLongFormOption and change the error message not specific to management agent. Incorporated above changes. Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) We can probably document that -D options will be overridden instead of emitting a warning. The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. Implementing this change requires lot of changes to argument parsing in native code and hence it is simpler to handle this at java layer. parseXmgmtArgs should be renamed as no longer -Xmgmt. Better not to tightly coupled with the option name. Ok. 291 if(args.startsWith("--start-management-agent")) { 292 return parseXmgmtArgs(args); 293 } What is this change for? This is required when management agent is started via attach mechanism. Mandy -Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 2/13/18 1:30 AM, Harsha Wardhana B wrote: Hi, Please find below the revised webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.02/ On Friday 09 February 2018 05:07 AM, mandy chung wrote: On 2/7/18 1:19 AM, Harsha Wardhana B wrote: > > --start-management-agent will not be recognized in the current format and > hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Yes. It requires lot more changes to launcher. Hence optional argument to --start-management-agent was not added in order to keep the launcher impact minimum. This is just one option for you to consider. So the current proposal of the new option to start a remote management server, right? Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) We can probably document that -D options will be overridden instead of emitting a warning. What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. Implementing this change requires lot of changes to argument parsing in native code and hence it is simpler to handle this at java layer. Can you describe how --start-management-agent option will be used for jcmd and any other tool that attaches to a running VM to start the agent? Example command-line will be useful. Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi, Below is the updated webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.03/ On Wednesday 14 February 2018 01:15 AM, mandy chung wrote: On 2/13/18 1:30 AM, Harsha Wardhana B wrote: Hi, Please find below the revised webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.02/ On Friday 09 February 2018 05:07 AM, mandy chung wrote: On 2/7/18 1:19 AM, Harsha Wardhana B wrote: > > --start-management-agent will not be recognized in the current format and > hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Yes. It requires lot more changes to launcher. Hence optional argument to --start-management-agent was not added in order to keep the launcher impact minimum. This is just one option for you to consider. So the current proposal of the new option to start a remote management server, right? Not necessarily. --start-management-agent=local=true starts local server and --start-management-agent=port=1234 starts remote management server. Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) We can probably document that -D options will be overridden instead of emitting a warning. What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. Implementing this change requires lot of changes to argument parsing in native code and hence it is simpler to handle this at java layer. Can you describe how --start-management-agent option will be used for jcmd and any other tool that attaches to a running VM to start the agent? Example command-line will be useful. --start-management-agent cannot be used by jcmd (or dynamic attach) as it accepts flags only in -D format or -D flags with com.sun.management removed. That code to parse string passed via jcmd was a mistake and hence I have removed it. Mandy -Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Ping. Could I please have reviews for below webrev. Thanks Harsha On Wednesday 14 February 2018 09:59 PM, Harsha Wardhana B wrote: Hi, Below is the updated webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.03/ On Wednesday 14 February 2018 01:15 AM, mandy chung wrote: On 2/13/18 1:30 AM, Harsha Wardhana B wrote: Hi, Please find below the revised webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.02/ On Friday 09 February 2018 05:07 AM, mandy chung wrote: On 2/7/18 1:19 AM, Harsha Wardhana B wrote: > > --start-management-agent will not be recognized in the current format and > hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Yes. It requires lot more changes to launcher. Hence optional argument to --start-management-agent was not added in order to keep the launcher impact minimum. This is just one option for you to consider. So the current proposal of the new option to start a remote management server, right? Not necessarily. --start-management-agent=local=true starts local server and --start-management-agent=port=1234 starts remote management server. Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) We can probably document that -D options will be overridden instead of emitting a warning. What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. Implementing this change requires lot of changes to argument parsing in native code and hence it is simpler to handle this at java layer. Can you describe how --start-management-agent option will be used for jcmd and any other tool that attaches to a running VM to start the agent? Example command-line will be useful. --start-management-agent cannot be used by jcmd (or dynamic attach) as it accepts flags only in -D format or -D flags with com.sun.management removed. That code to parse string passed via jcmd was a mistake and hence I have removed it. Mandy -Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
You are going backwards. You need to have a CSR approved first. Kumar Ping. Could I please have reviews for below webrev. Thanks Harsha On Wednesday 14 February 2018 09:59 PM, Harsha Wardhana B wrote: Hi, Below is the updated webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.03/ On Wednesday 14 February 2018 01:15 AM, mandy chung wrote: On 2/13/18 1:30 AM, Harsha Wardhana B wrote: Hi, Please find below the revised webrev. http://cr.openjdk.java.net/~hb/8187498/webrev.02/ On Friday 09 February 2018 05:07 AM, mandy chung wrote: On 2/7/18 1:19 AM, Harsha Wardhana B wrote: > > --start-management-agent will not be recognized in the current format and > hence will not default to --start-management-agent=local=true. `--start-local-management-server` is one option as Alan suggests. Another option is to make `--start-management-agent` to accept an optional argument. VM can accept `--start-management-agent` or `--start-management-agent=port=1234,ssl=on`. It may require more launcher change to support it. Yes. It requires lot more changes to launcher. Hence optional argument to --start-management-agent was not added in order to keep the launcher impact minimum. This is just one option for you to consider. So the current proposal of the new option to start a remote management server, right? Not necessarily. --start-management-agent=local=true starts local server and --start-management-agent=port=1234 starts remote management server. Agent.java If --start-management-agent is set, -Dcom.sun.management.* takes precedence. Do you really want to do that? The new VM option intends to simplify the command-line to type in. I think it's cleaner and reasonable if --start-management-agent is specified, -Dcom.sun.management.* are ignored (maybe worth emit a warning if set) We can probably document that -D options will be overridden instead of emitting a warning. What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. The implementation uses VMManagement::getVmArguments and scan the VM options for -start-management-agent. The VM is the one invoking jdk.internal.agent.Agent::startAgent. A simpler option is to change Agent::startAgent to take an argument parameter that will be passed with the argument of --start-management-agent or null if not set. Similarly for startRemoteManagementAgent and startLocalAgent_name. Implementing this change requires lot of changes to argument parsing in native code and hence it is simpler to handle this at java layer. Can you describe how --start-management-agent option will be used for jcmd and any other tool that attaches to a running VM to start the agent? Example command-line will be useful. --start-management-agent cannot be used by jcmd (or dynamic attach) as it accepts flags only in -D format or -D flags with com.sun.management removed. That code to parse string passed via jcmd was a mistake and hence I have removed it. Mandy -Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
The code review and CSR review can be in parallel. For this case, I agree with Kumar to have CSR written that would help the code review. Please specify the behavior and its relationship with jcmd and other relevant diagnosability tools. On 2/20/18 6:41 AM, Kumar Srinivasan wrote: What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. VM options are the last one wins if same option specified multiple times. In this case, it could cause confusion (the last -D option sets the value to 3456 but it's set to a different value). Why not taking the simplest approach - when --start-management-agent is set, it does not accept mixing the old way (i.e. does not accept the management properties to be set via -D)? This RFE is to make the command-line simpler and ease-of-use. I don't see any downside to migrate entirely to the new form. Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On Wednesday 21 February 2018 01:51 AM, mandy chung wrote: The code review and CSR review can be in parallel. For this case, I agree with Kumar to have CSR written that would help the code review. Please specify the behavior and its relationship with jcmd and other relevant diagnosability tools. ok. On 2/20/18 6:41 AM, Kumar Srinivasan wrote: What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. VM options are the last one wins if same option specified multiple times. In this case, it could cause confusion (the last -D option sets the value to 3456 but it's set to a different value). Why not taking the simplest approach - when --start-management-agent is set, it does not accept mixing the old way (i.e. does not accept the management properties to be set via -D)? This RFE is to make the command-line simpler and ease-of-use. I don't see any downside to migrate entirely to the new form. We cannot get rid of specifying options via -D. We have plenty of -D flags but very few have short-hand alternative via --start-management-agent. If management properties are specified by --start-management-agent, the options specified by -D are anyway overwritten if specified. Mandy Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
Hi, I'm a bit leary of command line arguments being special cased and the corresponding custom mappings to system properties. The convenience is fine but we need to keep the handling out of native code so it is easier to maintain. We don't have a Java API for processing (VM) command line arguments so they are being shoehorned into properties. $.02, Roger On 2/21/2018 12:55 AM, Harsha Wardhana B wrote: On Wednesday 21 February 2018 01:51 AM, mandy chung wrote: The code review and CSR review can be in parallel. For this case, I agree with Kumar to have CSR written that would help the code review. Please specify the behavior and its relationship with jcmd and other relevant diagnosability tools. ok. On 2/20/18 6:41 AM, Kumar Srinivasan wrote: What is the behavior when -Dcom.sun.management.jmxremote.port=1234 --start-management-agent port=2345 -Dcom.sun.management.jmxremote.port=3456? What is the value of the system property com.sun.management.jmxremote.port at runtime? What port number does the management server start with? As said earlier, values set via new flags override values set by -D flags. So 2345 will be the value of com.sun.management.jmxremote.port. Added a test case to validate that. VM options are the last one wins if same option specified multiple times. In this case, it could cause confusion (the last -D option sets the value to 3456 but it's set to a different value). Why not taking the simplest approach - when --start-management-agent is set, it does not accept mixing the old way (i.e. does not accept the management properties to be set via -D)? This RFE is to make the command-line simpler and ease-of-use. I don't see any downside to migrate entirely to the new form. We cannot get rid of specifying options via -D. We have plenty of -D flags but very few have short-hand alternative via --start-management-agent. If management properties are specified by --start-management-agent, the options specified by -D are anyway overwritten if specified. Mandy Harsha
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On 2/20/18 9:55 PM, Harsha Wardhana B wrote: We cannot get rid of specifying options via -D. We have plenty of -D flags but very few have short-hand alternative via --start-management-agent. If management properties are specified by --start-management-agent, the options specified by -D are anyway overwritten if specified. I have to see the specification of this feature and give further feedback. If the new option does not accept all management properties, what is the benefit of this new option? Mandy
Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties
On Wednesday 21 February 2018 09:51 PM, mandy chung wrote: On 2/20/18 9:55 PM, Harsha Wardhana B wrote: We cannot get rid of specifying options via -D. We have plenty of -D flags but very few have short-hand alternative via --start-management-agent. If management properties are specified by --start-management-agent, the options specified by -D are anyway overwritten if specified. I have to see the specification of this feature and give further feedback. If the new option does not accept all management properties, what is the benefit of this new option? The new options makes it easy to start and configure default agent. As of now, users needs to be aware of -D flags which are long and cumbersome. Hence only most frequently used -D flags are given the short-hand alternative to --start-management-agent. Harsha Mandy