Thank you Chris and Serguei for reviewing this change! Best regards, Daniil
On 2/5/20, 11:15 AM, "Chris Plummer" <chris.plum...@oracle.com> wrote: +1 Chris On 2/5/20 9:36 AM, serguei.spit...@oracle.com wrote: > Hi Daniil, > > Looks good. > Thank you for the update! > > Thanks, > Serguei > > > On 2/4/20 22:00, Daniil Titov wrote: >> Hi Serguei, >> >> Thank you for finding this! Please review the new version of webrev [1] >> that has it corrected. The new webrev also includes changes in the >> test to >> make sure that all jstatd tests run for both styles of command line >> options. >> Testing: Mach5 jobs for sun/tools/jstatd succeeded. Tiers1, >> tiers2, tiers3, >> and tiers5 job are in the progress. >> >> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.02/ >> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729 >> [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357 >> >> Thanks, >> Daniil >> >> From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> >> Date: Tuesday, February 4, 2020 at 7:51 PM >> To: Daniil Titov <daniil.x.ti...@oracle.com>, >> "serviceability-dev@openjdk.java.net" >> <serviceability-dev@openjdk.java.net> >> Subject: Re: RFR: 8196729: Add jstatd option to specify RMI connector >> port >> >> Hi Daniil, >> >> It looks okay to me in general. >> But I'm puzzled with this part: >> >> http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java.udiff.html >> >> + } else if (arg.startsWith("-r")) { >> + if (arg.compareTo("-r") != 0) { >> + port = Integer.parseInt(arg.substring(2)); >> + } else { >> + argc++; >> + if (argc >= args.length) { >> + printUsage(); >> + System.exit(1); >> + } >> + rmiPort = Integer.parseInt(args[argc]); >> + } >> >> The option -r is for rmi connection port number. >> Why does this code set the RMI registry port? : >> + if (arg.compareTo("-r") != 0) { >> + port = Integer.parseInt(arg.substring(2)); >> >> Thanks, >> Serguei >> >> >> On 1/31/20 13:08, Daniil Titov wrote: >> Please review change [1] that adds a new command line option to >> jstatd tool to specify a RMI connector port. >> >> Currently a random port is used that prevents this tool from being >> used behind a firewall or in a container. >> >> New CSR [3] was created for this change and it needs to be reviewed >> as well. >> >> Man pages for jstatd will be updated in a separate issue. >> >> Testing: Mach5 tier1-tier3 and sun/tools/jstatd/* tests succeeded. >> Mach5 tier5 tests are in progress. >> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/ >> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729 >> [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357 >> >> Thank you, >> Daniil >> >> >> >> >> >> >> >