Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Thank you Chris and Serguei for reviewing this change!
Best regards,
Daniil
On 2/5/20, 11:15 AM, "Chris Plummer" 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"
>> Date: Tuesday, February 4, 2020 at 7:51 PM
>> To: Daniil Titov ,
>> "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
>>
>>
>>
>>
>>
>>
>>
>
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
+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"
Date: Tuesday, February 4, 2020 at 7:51 PM
To: Daniil Titov ,
"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
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
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"
Date: Tuesday, February 4, 2020 at 7:51 PM
To: Daniil Titov , "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
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
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"
Date: Tuesday, February 4, 2020 at 7:51 PM
To: Daniil Titov ,
"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
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Good catch. It's a copy-n-paste bug from the block of code just above
this block. You can use "-r " or "-r". The buggy code is
handling the second form. The test case uses the first form so didn't
catch this error.
Chris
On 2/4/20 7:51 PM, serguei.spit...@oracle.com wrote:
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
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
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Daniil, Ok. Changes look good. thanks, Chris On 1/31/20 2:14 PM, Daniil Titov wrote: Hi Chris, Thank you for describing this in such details! Your description is correct. In addition, apart from jstat there is jps tool that also can communicate with jstatd and currently it faces the same problems if jstatd is deployed behind a firewall or in a container: after successful connection to RMI registry the further communication fails since jstatd chooses a random RMI port that is not published by the container or might be blocked by a firewall configuration. Best regards, Daniil On 1/31/20, 1:45 PM, "Chris Plummer" wrote: Hi Daniil, Just want to make sure I understand what communications are going on here. Your concern is when the jstat and jstatd processes are on different sides of the firewall. When you launch jstatd, you specify the socket port it will receive requests on, and when you launch jstat, you must specify this same socket port, so no firewall problem there assuming the firewall is configured to allow communication over that port. However, once the request is received by jstatd, data can be communicated via RMI rather than over the specified socket port. By default jstatd was choosing a random RMI port, and I assume this RMI port was communicated to the jstat process via the initial socket port. This presents a problem for firewall configuration, since the firewall configuration cannot know the RMI port that will be used. So now you're allowing the rmi port to also be specified. Am I close? :) Chris On 1/31/20 1:08 PM, 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 > > >
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Chris, Thank you for describing this in such details! Your description is correct. In addition, apart from jstat there is jps tool that also can communicate with jstatd and currently it faces the same problems if jstatd is deployed behind a firewall or in a container: after successful connection to RMI registry the further communication fails since jstatd chooses a random RMI port that is not published by the container or might be blocked by a firewall configuration. Best regards, Daniil On 1/31/20, 1:45 PM, "Chris Plummer" wrote: Hi Daniil, Just want to make sure I understand what communications are going on here. Your concern is when the jstat and jstatd processes are on different sides of the firewall. When you launch jstatd, you specify the socket port it will receive requests on, and when you launch jstat, you must specify this same socket port, so no firewall problem there assuming the firewall is configured to allow communication over that port. However, once the request is received by jstatd, data can be communicated via RMI rather than over the specified socket port. By default jstatd was choosing a random RMI port, and I assume this RMI port was communicated to the jstat process via the initial socket port. This presents a problem for firewall configuration, since the firewall configuration cannot know the RMI port that will be used. So now you're allowing the rmi port to also be specified. Am I close? :) Chris On 1/31/20 1:08 PM, 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 > > >
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Daniil, Just want to make sure I understand what communications are going on here. Your concern is when the jstat and jstatd processes are on different sides of the firewall. When you launch jstatd, you specify the socket port it will receive requests on, and when you launch jstat, you must specify this same socket port, so no firewall problem there assuming the firewall is configured to allow communication over that port. However, once the request is received by jstatd, data can be communicated via RMI rather than over the specified socket port. By default jstatd was choosing a random RMI port, and I assume this RMI port was communicated to the jstat process via the initial socket port. This presents a problem for firewall configuration, since the firewall configuration cannot know the RMI port that will be used. So now you're allowing the rmi port to also be specified. Am I close? :) Chris On 1/31/20 1:08 PM, 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
