Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa and Serguei,
Thank you for reviewing this change.
Best regards,
--Daniil
On 3/25/20, 1:01 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
On 3/24/20 10:00, Daniil Titov wrote:
> Hi Serguei,
>
>> It looks like you removed the last call site of DebugServer.main.
> Yes. It is correct.
>
>> Do we need to remove the DebugServer.java as well?
> I was considering this but since it is a public class I think it needs to
be deprecated first. I also think that it would be better to do in a separate
issue
> since a CSR for deprecation needs to be filed for that. If you agree I
will create a new issue for that.
I'm okay to separate this.
Thanks,
Serguei
>
> Thanks,
> Daniil
>
>
> On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com"
wrote:
>
> Hi Daniil,
>
> It looks pretty good in general.
>
> It looks like you removed the last call site of DebugServer.main.
> Do we need to remove the DebugServer.java as well?
>
> Thanks,
> Serguei
>
>
> On 3/22/20 15:29, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the webrev that merges
SADebugDTest.java with changes done in [2].
> >
> > Also the CRS [3] and the help message for debug server in
SALauncher.java were updated to specify that '--hostname'
> > option could be a hostname or an IPv4/IPv6 address.
> >
> > > Ok, but I think it might be more simply with TestLibrary.
> > > For example, can you use TestLibrary::getUnusedRandomPort ?
It is used in test/jdk/java/rmi/testlibrary/RMID.java .
> >
> > TestLibrary:: getUnusedRandomPort() doesn't allow to specify what
ports are reserved and it uses some hardcoded port range [FIXED_PORT_MIN,
FIXED_PORT_MAX] as reserved ports. Besides,
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
> >
> > Nevertheless, to simplify the test itself I moved
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to
jdk.test.lib.Utils in /test/lib.
> >
> > Testing: Mach5 tier1-tier3 tests (that include
serviceability/sa/sadebugd tests) succeeded.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8238268
> > [3] https://bugs.openjdk.java.net/browse/JDK-8239831
> >
> > Thank you,
> > Daniil
> >
> > On 3/13/20, 7:23 PM, "Yasumasa Suenaga"
wrote:
> >
> > Hi Daniil,
> >
> > On 2020/03/14 7:05, Daniil Titov wrote:
> > > Hi Yasumasa, Serguei and Alex,
> > >
> > > Please review a new version of the webrev that includes the
changes Yasumasa suggested.
> > >
> > >> Shutdown hook is already registered in c'tor of
HotSpotAgent.
> > >> It works same as shutdownServer(), so I think shutdown
hook at SALauncher is not needed.
> > >
> > > The shutdown hook registered in the HotSpotAgent c'tor only
works for non-servers, so we still need a
> > > the shutdown hook for remote server being added in
SALauncher. I changed it to use the lambda expression.
> > >
> > > 101 public HotSpotAgent() {
> > > 102 // for non-server add shutdown hook to
clean-up debugger in case
> > > 103 // of forced exit. For remote server,
shutdown hook is added by
> > > 104 // DebugServer.
> > > 105 Runtime.getRuntime().addShutdownHook(new
java.lang.Thread(
> > > 106 new Runnable() {
> > > 107 public void run() {
> > > 108 synchronized (HotSpotAgent.this) {
> > > 109 if (!isServer) {
> > > 110 detach();
> > > 111 }
> > > 112 }
> > > 113 }
> > > 114 }));
> > > 115 }
> >
> > I missed it, thanks!
> >
> >
> > >>> Hmm... I think port check (already in use) is not
needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties
contains
> > >>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> > > As I understand exclusiveAccess.dirs prevents only the
tests located in this directory from being run simulta
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil,
On 3/24/20 10:00, Daniil Titov wrote:
Hi Serguei,
It looks like you removed the last call site of DebugServer.main.
Yes. It is correct.
Do we need to remove the DebugServer.java as well?
I was considering this but since it is a public class I think it needs to be
deprecated first. I also think that it would be better to do in a separate issue
since a CSR for deprecation needs to be filed for that. If you agree I will
create a new issue for that.
I'm okay to separate this.
Thanks,
Serguei
Thanks,
Daniil
On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
It looks pretty good in general.
It looks like you removed the last call site of DebugServer.main.
Do we need to remove the DebugServer.java as well?
Thanks,
Serguei
On 3/22/20 15:29, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that merges SADebugDTest.java
with changes done in [2].
>
> Also the CRS [3] and the help message for debug server in
SALauncher.java were updated to specify that '--hostname'
> option could be a hostname or an IPv4/IPv6 address.
>
> > Ok, but I think it might be more simply with TestLibrary.
> > For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
> TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports
are reserved and it uses some hardcoded port range [FIXED_PORT_MIN,
FIXED_PORT_MAX] as reserved ports. Besides,
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
>
> Nevertheless, to simplify the test itself I moved
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to
jdk.test.lib.Utils in /test/lib.
>
> Testing: Mach5 tier1-tier3 tests (that include
serviceability/sa/sadebugd tests) succeeded.
>
> [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
> [2] https://bugs.openjdk.java.net/browse/JDK-8238268
> [3] https://bugs.openjdk.java.net/browse/JDK-8239831
>
> Thank you,
> Daniil
>
> On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/14 7:05, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the webrev that includes the
changes Yasumasa suggested.
> >
> >> Shutdown hook is already registered in c'tor of HotSpotAgent.
> >> It works same as shutdownServer(), so I think shutdown hook
at SALauncher is not needed.
> >
> > The shutdown hook registered in the HotSpotAgent c'tor only works
for non-servers, so we still need a
> > the shutdown hook for remote server being added in SALauncher. I
changed it to use the lambda expression.
> >
> > 101 public HotSpotAgent() {
> > 102 // for non-server add shutdown hook to clean-up
debugger in case
> > 103 // of forced exit. For remote server, shutdown hook
is added by
> > 104 // DebugServer.
> > 105 Runtime.getRuntime().addShutdownHook(new
java.lang.Thread(
> > 106 new Runnable() {
> > 107 public void run() {
> > 108 synchronized (HotSpotAgent.this) {
> > 109 if (!isServer) {
> > 110 detach();
> > 111 }
> > 112 }
> > 113 }
> > 114 }));
> > 115 }
>
> I missed it, thanks!
>
>
> >>> Hmm... I think port check (already in use) is not needed
because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
> >>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> > As I understand exclusiveAccess.dirs prevents only the tests
located in this directory from being run simultaneously and other tests could still
run in parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of local
arrays.
>
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
>
> Thanks,
>
> Yasumasa
>
>
> > Testing: Mach5 tier1-tier3 tests (that include
serviceability/sa/sadebugd tests) succeeded.
> >
> > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei,
>It looks like you removed the last call site of DebugServer.main.
Yes. It is correct.
>Do we need to remove the DebugServer.java as well?
I was considering this but since it is a public class I think it needs to be
deprecated first. I also think that it would be better to do in a separate issue
since a CSR for deprecation needs to be filed for that. If you agree I will
create a new issue for that.
Thanks,
Daniil
On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
It looks pretty good in general.
It looks like you removed the last call site of DebugServer.main.
Do we need to remove the DebugServer.java as well?
Thanks,
Serguei
On 3/22/20 15:29, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that merges SADebugDTest.java
with changes done in [2].
>
> Also the CRS [3] and the help message for debug server in SALauncher.java
were updated to specify that '--hostname'
> option could be a hostname or an IPv4/IPv6 address.
>
> > Ok, but I think it might be more simply with TestLibrary.
> > For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
> TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports
are reserved and it uses some hardcoded port range [FIXED_PORT_MIN,
FIXED_PORT_MAX] as reserved ports. Besides,
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
>
> Nevertheless, to simplify the test itself I moved
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to
jdk.test.lib.Utils in /test/lib.
>
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
>
> [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
> [2] https://bugs.openjdk.java.net/browse/JDK-8238268
> [3] https://bugs.openjdk.java.net/browse/JDK-8239831
>
> Thank you,
> Daniil
>
> On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/14 7:05, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the webrev that includes the
changes Yasumasa suggested.
> >
> >> Shutdown hook is already registered in c'tor of HotSpotAgent.
> >> It works same as shutdownServer(), so I think shutdown hook
at SALauncher is not needed.
> >
> > The shutdown hook registered in the HotSpotAgent c'tor only works
for non-servers, so we still need a
> > the shutdown hook for remote server being added in SALauncher. I
changed it to use the lambda expression.
> >
> > 101 public HotSpotAgent() {
> > 102 // for non-server add shutdown hook to clean-up
debugger in case
> > 103 // of forced exit. For remote server, shutdown hook
is added by
> > 104 // DebugServer.
> > 105 Runtime.getRuntime().addShutdownHook(new
java.lang.Thread(
> > 106 new Runnable() {
> > 107 public void run() {
> > 108 synchronized (HotSpotAgent.this) {
> > 109 if (!isServer) {
> > 110 detach();
> > 111 }
> > 112 }
> > 113 }
> > 114 }));
> > 115 }
>
> I missed it, thanks!
>
>
> >>> Hmm... I think port check (already in use) is not needed
because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
> >>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> > As I understand exclusiveAccess.dirs prevents only the tests
located in this directory from being run simultaneously and other tests could
still run in parallel with one of these tests. Thus I would prefer to have the
retry mechanism in place. I simplified the code using the class variables
instead of local arrays.
>
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
>
> Thanks,
>
> Yasumasa
>
>
> > Testing: Mach5 tier1-tier3 tests (that include
serviceability/sa/sadebugd tests) succeeded.
> >
> > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
> > [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> > [3] Bug: https://bugs.openjdk.java.net/browse/
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil,
It looks pretty good in general.
It looks like you removed the last call site of DebugServer.main.
Do we need to remove the DebugServer.java as well?
Thanks,
Serguei
On 3/22/20 15:29, Daniil Titov wrote:
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that merges SADebugDTest.java with
changes done in [2].
Also the CRS [3] and the help message for debug server in SALauncher.java were
updated to specify that '--hostname'
option could be a hostname or an IPv4/IPv6 address.
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports are
reserved and it uses some hardcoded port range [FIXED_PORT_MIN, FIXED_PORT_MAX]
as reserved ports. Besides, test/jdk/java/rmi/testlibrary/TestLibrary.java
class cannot be directly used in test/hotspot/jtreg/serviceability/* tests (it
doesn't compile).
Nevertheless, to simplify the test itself I moved findUnreservedFreePort(int ..
reservedPorts) from SADebugTest.java to jdk.test.lib.Utils in /test/lib.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8238268
[3] https://bugs.openjdk.java.net/browse/JDK-8239831
Thank you,
Daniil
On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/14 7:05, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that includes the changes
Yasumasa suggested.
>
>> Shutdown hook is already registered in c'tor of HotSpotAgent.
>> It works same as shutdownServer(), so I think shutdown hook at
SALauncher is not needed.
>
> The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
> the shutdown hook for remote server being added in SALauncher. I changed
it to use the lambda expression.
>
> 101 public HotSpotAgent() {
> 102 // for non-server add shutdown hook to clean-up debugger
in case
> 103 // of forced exit. For remote server, shutdown hook is
added by
> 104 // DebugServer.
> 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
> 106 new Runnable() {
> 107 public void run() {
> 108 synchronized (HotSpotAgent.this) {
> 109 if (!isServer) {
> 110 detach();
> 111 }
> 112 }
> 113 }
> 114 }));
> 115 }
I missed it, thanks!
>>> Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> As I understand exclusiveAccess.dirs prevents only the tests located in
this directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry mechanism
in place. I simplified the code using the class variables instead of local arrays.
Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
Thanks,
Yasumasa
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
>
> Thank you,
> Daniil
>
> On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/07 3:38, Daniil Titov wrote:
> > Hi Yasumasa,
> >
> > -> checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> > I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect ( it
throws an exception if parameters are incorrect) and ignores its return might look confusing.
Thus, I found it more appropriate to wrap it inside a method with more relevant name .
>
> Ok, but I prefer to leave comment it.
>
>
> > > SADebugDTest
> > > - Why do you declare portInUse and testResult as array?
Their length is 1, so I think you don't need to use array.
> > We cannot use primitives there since these loc
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil,
Looks good!
Yasumasa
On 2020/03/23 7:29, Daniil Titov wrote:
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that merges SADebugDTest.java with
changes done in [2].
Also the CRS [3] and the help message for debug server in SALauncher.java were
updated to specify that '--hostname'
option could be a hostname or an IPv4/IPv6 address.
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports are
reserved and it uses some hardcoded port range [FIXED_PORT_MIN, FIXED_PORT_MAX]
as reserved ports. Besides, test/jdk/java/rmi/testlibrary/TestLibrary.java
class cannot be directly used in test/hotspot/jtreg/serviceability/* tests (it
doesn't compile).
Nevertheless, to simplify the test itself I moved findUnreservedFreePort(int ..
reservedPorts) from SADebugTest.java to jdk.test.lib.Utils in /test/lib.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8238268
[3] https://bugs.openjdk.java.net/browse/JDK-8239831
Thank you,
Daniil
On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/14 7:05, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that includes the changes
Yasumasa suggested.
>
>> Shutdown hook is already registered in c'tor of HotSpotAgent.
>> It works same as shutdownServer(), so I think shutdown hook at
SALauncher is not needed.
>
> The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
> the shutdown hook for remote server being added in SALauncher. I changed
it to use the lambda expression.
>
> 101 public HotSpotAgent() {
> 102 // for non-server add shutdown hook to clean-up debugger
in case
> 103 // of forced exit. For remote server, shutdown hook is
added by
> 104 // DebugServer.
> 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
> 106 new Runnable() {
> 107 public void run() {
> 108 synchronized (HotSpotAgent.this) {
> 109 if (!isServer) {
> 110 detach();
> 111 }
> 112 }
> 113 }
> 114 }));
> 115 }
I missed it, thanks!
>>> Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> As I understand exclusiveAccess.dirs prevents only the tests located in
this directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry mechanism
in place. I simplified the code using the class variables instead of local arrays.
Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
Thanks,
Yasumasa
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
>
> Thank you,
> Daniil
>
> On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/07 3:38, Daniil Titov wrote:
> > Hi Yasumasa,
> >
> > -> checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> > I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect ( it
throws an exception if parameters are incorrect) and ignores its return might look confusing.
Thus, I found it more appropriate to wrap it inside a method with more relevant name .
>
> Ok, but I prefer to leave comment it.
>
>
> > > SADebugDTest
> > > - Why do you declare portInUse and testResult as array?
Their length is 1, so I think you don't need to use array.
> > We cannot use primitives there since these local variables are
captured in lambda expression and are required to be final.
> > The other option is to use some other wrapper for t
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that merges SADebugDTest.java with
changes done in [2].
Also the CRS [3] and the help message for debug server in SALauncher.java were
updated to specify that '--hostname'
option could be a hostname or an IPv4/IPv6 address.
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
> test/jdk/java/rmi/testlibrary/RMID.java .
TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports are
reserved and it uses some hardcoded port range [FIXED_PORT_MIN, FIXED_PORT_MAX]
as reserved ports. Besides, test/jdk/java/rmi/testlibrary/TestLibrary.java
class cannot be directly used in test/hotspot/jtreg/serviceability/* tests (it
doesn't compile).
Nevertheless, to simplify the test itself I moved findUnreservedFreePort(int ..
reservedPorts) from SADebugTest.java to jdk.test.lib.Utils in /test/lib.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8238268
[3] https://bugs.openjdk.java.net/browse/JDK-8239831
Thank you,
Daniil
On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/14 7:05, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that includes the changes
Yasumasa suggested.
>
>> Shutdown hook is already registered in c'tor of HotSpotAgent.
>> It works same as shutdownServer(), so I think shutdown hook at
SALauncher is not needed.
>
> The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
> the shutdown hook for remote server being added in SALauncher. I changed
it to use the lambda expression.
>
> 101 public HotSpotAgent() {
> 102 // for non-server add shutdown hook to clean-up debugger in
case
> 103 // of forced exit. For remote server, shutdown hook is
added by
> 104 // DebugServer.
> 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
> 106 new Runnable() {
> 107 public void run() {
> 108 synchronized (HotSpotAgent.this) {
> 109 if (!isServer) {
> 110 detach();
> 111 }
> 112 }
> 113 }
> 114 }));
> 115 }
I missed it, thanks!
>>> Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> As I understand exclusiveAccess.dirs prevents only the tests located in
this directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
Thanks,
Yasumasa
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
>
> Thank you,
> Daniil
>
> On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/07 3:38, Daniil Titov wrote:
> > Hi Yasumasa,
> >
> > -> checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> > I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect (
it throws an exception if parameters are incorrect) and ignores its return
might look confusing. Thus, I found it more appropriate to wrap it inside a
method with more relevant name .
>
> Ok, but I prefer to leave comment it.
>
>
> > > SADebugDTest
> > > - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
> > We cannot use primitives there since these local variables are
captured in lambda expression and are required to be final.
> > The other option is to use some other wrapper for them but I don't
see any obvious benefits in it comparing to the array.
>
> Hmm... I think po
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil,
The update looks pretty good to me so far.
I'll make another pass tomorrow.
Thanks,
Serguei
On 3/13/20 15:05, Daniil Titov wrote:
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that includes the changes Yasumasa
suggested.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
the shutdown hook for remote server being added in SALauncher. I changed it to
use the lambda expression.
101 public HotSpotAgent() {
102 // for non-server add shutdown hook to clean-up debugger in case
103 // of forced exit. For remote server, shutdown hook is added by
104 // DebugServer.
105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
106 new Runnable() {
107 public void run() {
108 synchronized (HotSpotAgent.this) {
109 if (!isServer) {
110 detach();
111 }
112 }
113 }
114 }));
115 }
Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
`exclusiveAccess.dirs=.` to avoid concurrent execution
As I understand exclusiveAccess.dirs prevents only the tests located in this
directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
Thank you,
Daniil
On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/07 3:38, Daniil Titov wrote:
> Hi Yasumasa,
>
> -> checkBasicOptions() is needed? I think you can remove this method
and embed it in caller.
> I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect ( it
throws an exception if parameters are incorrect) and ignores its return might look
confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant
name .
Ok, but I prefer to leave comment it.
> > SADebugDTest
> > - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
> We cannot use primitives there since these local variables are captured
in lambda expression and are required to be final.
> The other option is to use some other wrapper for them but I don't see
any obvious benefits in it comparing to the array.
Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains `exclusiveAccess.dirs=.` to avoid concurrent execution.
If you do not think this error check, test code is more simply.
> I will include your other suggestion in the new version of the webrev.
Sorry, I have one more comment:
> - Shutdown hook is very good idea. You can implement more simply if you use lambda expression.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
Thanks,
Yasumasa
> Thanks!
> Daniil
>
> On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
>
> - SALauncher.java
> - checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> - I think registryPort should be checked with
Integer.parseInt() like others (rmiPort and pid) rather than regex.
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
>
> - SADebugDTest.java
> - Please add bug ID to @bug.
> - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/03/06 10:15, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the fix [1] that addresses your
comments. The new version in addition to RMI connector
> > port option introduces two more options to specify
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil,
On 2020/03/14 7:05, Daniil Titov wrote:
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that includes the changes Yasumasa
suggested.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
the shutdown hook for remote server being added in SALauncher. I changed it to
use the lambda expression.
101 public HotSpotAgent() {
102 // for non-server add shutdown hook to clean-up debugger in case
103 // of forced exit. For remote server, shutdown hook is added by
104 // DebugServer.
105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
106 new Runnable() {
107 public void run() {
108 synchronized (HotSpotAgent.this) {
109 if (!isServer) {
110 detach();
111 }
112 }
113 }
114 }));
115 }
I missed it, thanks!
Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
`exclusiveAccess.dirs=.` to avoid concurrent execution
As I understand exclusiveAccess.dirs prevents only the tests located in this
directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
Thanks,
Yasumasa
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
Thank you,
Daniil
On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/07 3:38, Daniil Titov wrote:
> Hi Yasumasa,
>
> -> checkBasicOptions() is needed? I think you can remove this method
and embed it in caller.
> I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect ( it
throws an exception if parameters are incorrect) and ignores its return might look
confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant
name .
Ok, but I prefer to leave comment it.
> > SADebugDTest
> > - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
> We cannot use primitives there since these local variables are captured
in lambda expression and are required to be final.
> The other option is to use some other wrapper for them but I don't see
any obvious benefits in it comparing to the array.
Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains `exclusiveAccess.dirs=.` to avoid concurrent execution.
If you do not think this error check, test code is more simply.
> I will include your other suggestion in the new version of the webrev.
Sorry, I have one more comment:
> - Shutdown hook is very good idea. You can implement more simply if you use lambda expression.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
Thanks,
Yasumasa
> Thanks!
> Daniil
>
> On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
>
> - SALauncher.java
> - checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> - I think registryPort should be checked with
Integer.parseInt() like others (rmiPort and pid) rather than regex.
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
>
> - SADebugDTest.java
> - Please add bug ID to @bug.
> - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/03/06 10:15, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the fix [1] that addre
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that includes the changes Yasumasa
suggested.
> Shutdown hook is already registered in c'tor of HotSpotAgent.
>It works same as shutdownServer(), so I think shutdown hook at SALauncher
> is not needed.
The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
the shutdown hook for remote server being added in SALauncher. I changed it to
use the lambda expression.
101 public HotSpotAgent() {
102 // for non-server add shutdown hook to clean-up debugger in case
103 // of forced exit. For remote server, shutdown hook is added by
104 // DebugServer.
105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
106 new Runnable() {
107 public void run() {
108 synchronized (HotSpotAgent.this) {
109 if (!isServer) {
110 detach();
111 }
112 }
113 }
114 }));
115 }
>>Hmm... I think port check (already in use) is not needed because
>> test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>> `exclusiveAccess.dirs=.` to avoid concurrent execution
As I understand exclusiveAccess.dirs prevents only the tests located in this
directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
Thank you,
Daniil
On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/07 3:38, Daniil Titov wrote:
> Hi Yasumasa,
>
> -> checkBasicOptions() is needed? I think you can remove this method
and embed it in caller.
> I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect (
it throws an exception if parameters are incorrect) and ignores its return
might look confusing. Thus, I found it more appropriate to wrap it inside a
method with more relevant name .
Ok, but I prefer to leave comment it.
> > SADebugDTest
> > - Why do you declare portInUse and testResult as array? Their length
is 1, so I think you don't need to use array.
> We cannot use primitives there since these local variables are captured
in lambda expression and are required to be final.
> The other option is to use some other wrapper for them but I don't see
any obvious benefits in it comparing to the array.
Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
`exclusiveAccess.dirs=.` to avoid concurrent execution.
If you do not think this error check, test code is more simply.
> I will include your other suggestion in the new version of the webrev.
Sorry, I have one more comment:
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
Thanks,
Yasumasa
> Thanks!
> Daniil
>
> On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
>
> - SALauncher.java
> - checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> - I think registryPort should be checked with
Integer.parseInt() like others (rmiPort and pid) rather than regex.
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
>
> - SADebugDTest.java
> - Please add bug ID to @bug.
> - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/03/06 10:15, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the fix [1] that addresses your
comments. The new version in addition to RMI connector
> > port option introduces two more options to specify RMI registry
port and RMI connector host name. Currently, these
> > last two settings could be specified using the system properties
but the system pro
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil, On 2020/03/07 3:38, Daniil Titov wrote: Hi Yasumasa, -> checkBasicOptions() is needed? I think you can remove this method and embed it in caller. I think that having a piece of code that invokes a method named "buildAttachArgs" with a copy of the argument map just for its side-effect ( it throws an exception if parameters are incorrect) and ignores its return might look confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant name . Ok, but I prefer to leave comment it. > SADebugDTest > - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array. We cannot use primitives there since these local variables are captured in lambda expression and are required to be final. The other option is to use some other wrapper for them but I don't see any obvious benefits in it comparing to the array. Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains `exclusiveAccess.dirs=.` to avoid concurrent execution. If you do not think this error check, test code is more simply. I will include your other suggestion in the new version of the webrev. Sorry, I have one more comment: - Shutdown hook is very good idea. You can implement more simply if you use lambda expression. Shutdown hook is already registered in c'tor of HotSpotAgent. It works same as shutdownServer(), so I think shutdown hook at SALauncher is not needed. Thanks, Yasumasa Thanks! Daniil On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote: Hi Daniil, - SALauncher.java - checkBasicOptions() is needed? I think you can remove this method and embed it in caller. - I think registryPort should be checked with Integer.parseInt() like others (rmiPort and pid) rather than regex. - Shutdown hook is very good idea. You can implement more simply if you use lambda expression. - SADebugDTest.java - Please add bug ID to @bug. - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array. Thanks, Yasumasa On 2020/03/06 10:15, Daniil Titov wrote: > Hi Yasumasa, Serguei and Alex, > > Please review a new version of the fix [1] that addresses your comments. The new version in addition to RMI connector > port option introduces two more options to specify RMI registry port and RMI connector host name. Currently, these > last two settings could be specified using the system properties but the system properties have the following disadvantages > comparing to the command line options: > - It’s hard to know about them: they are not listed in tool’s help. > - They have long names that hard to remember > - It is easy to mistype them in the command line and you will not get any warning about it. > > The CSR [2] was also updated and needs to be reviewed. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/ > [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > Thank you, > Daniil > > On 2/24/20, 5:45 AM, "Yasumasa Suenaga" wrote: > > Hi Daniil, > > - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments. > Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply. > > - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used. > But you can use same port number as RMI registry (1099). > It is same as relation between jmxremote.port and jmxremote.rmi.port. > > > Thanks, > > Yasumasa > > > On 2020/02/24 13:21, Daniil Titov wrote: > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, -> checkBasicOptions() is needed? I think you can remove this method and embed it in caller. I think that having a piece of code that invokes a method named "buildAttachArgs" with a copy of the argument map just for its side-effect ( it throws an exception if parameters are incorrect) and ignores its return might look confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant name . > SADebugDTest > - Why do you declare portInUse and testResult as array? Their length is 1, > so I think you don't need to use array. We cannot use primitives there since these local variables are captured in lambda expression and are required to be final. The other option is to use some other wrapper for them but I don't see any obvious benefits in it comparing to the array. I will include your other suggestion in the new version of the webrev. Thanks! Daniil On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote: Hi Daniil, - SALauncher.java - checkBasicOptions() is needed? I think you can remove this method and embed it in caller. - I think registryPort should be checked with Integer.parseInt() like others (rmiPort and pid) rather than regex. - Shutdown hook is very good idea. You can implement more simply if you use lambda expression. - SADebugDTest.java - Please add bug ID to @bug. - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array. Thanks, Yasumasa On 2020/03/06 10:15, Daniil Titov wrote: > Hi Yasumasa, Serguei and Alex, > > Please review a new version of the fix [1] that addresses your comments. The new version in addition to RMI connector > port option introduces two more options to specify RMI registry port and RMI connector host name. Currently, these > last two settings could be specified using the system properties but the system properties have the following disadvantages > comparing to the command line options: > - It’s hard to know about them: they are not listed in tool’s help. > - They have long names that hard to remember > - It is easy to mistype them in the command line and you will not get any warning about it. > > The CSR [2] was also updated and needs to be reviewed. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/ > [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > Thank you, > Daniil > > On 2/24/20, 5:45 AM, "Yasumasa Suenaga" wrote: > > Hi Daniil, > > - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments. > Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply. > > - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used. > But you can use same port number as RMI registry (1099). > It is same as relation between jmxremote.port and jmxremote.rmi.port. > > > Thanks, > > Yasumasa > > > On 2020/02/24 13:21, Daniil Titov wrote: > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > > > >// delegate to the actual SA debug server. > > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > > and sun.jvm.hotspot.DebugServer and delegating the call. With this change
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil, Okay, thanks! Serguei On 2/25/20 11:38 AM, Daniil Titov wrote: Hi Serguei, I will update the CSR and the fix to include this change. Thank you, Daniil On 2/25/20, 11:07 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, Thank you for reply. I agree with the approach to avoid using system properties. Then it is better to be consistent. I'd consider adding an RMI registry port option as well. Will look at your comments in the CSR and reply there. Thanks, Serguei On 2/25/20 10:05 AM, Daniil Titov wrote: > Hi Serguei, > > I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid > using the system properties and prefer the command line options instead. > > Thanks, > Daniil > > On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: > > Hi Daniil, > > I've looked at CSR and posted a couple of questions there. > It'd be nice if you help to resolve my confusion. :) > > Thanks, > Serguei > > > On 2/23/20 20:21, Daniil Titov wrote: > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > > > >// delegate to the actual SA debug server. > > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > > but I would prefer to address it in a separate issue. > > > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > > container and connecting to it with the GUI debugger. > > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > > > Thank you, > > Daniil > > > > > > > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei, I will update the CSR and the fix to include this change. Thank you, Daniil On 2/25/20, 11:07 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, Thank you for reply. I agree with the approach to avoid using system properties. Then it is better to be consistent. I'd consider adding an RMI registry port option as well. Will look at your comments in the CSR and reply there. Thanks, Serguei On 2/25/20 10:05 AM, Daniil Titov wrote: > Hi Serguei, > > I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid > using the system properties and prefer the command line options instead. > > Thanks, > Daniil > > On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: > > Hi Daniil, > > I've looked at CSR and posted a couple of questions there. > It'd be nice if you help to resolve my confusion. :) > > Thanks, > Serguei > > > On 2/23/20 20:21, Daniil Titov wrote: > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > > > >// delegate to the actual SA debug server. > > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > > but I would prefer to address it in a separate issue. > > > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > > container and connecting to it with the GUI debugger. > > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > > > Thank you, > > Daniil > > > > > > > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil, Thank you for reply. I agree with the approach to avoid using system properties. Then it is better to be consistent. I'd consider adding an RMI registry port option as well. Will look at your comments in the CSR and reply there. Thanks, Serguei On 2/25/20 10:05 AM, Daniil Titov wrote: Hi Serguei, I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid using the system properties and prefer the command line options instead. Thanks, Daniil On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, I've looked at CSR and posted a couple of questions there. It'd be nice if you help to resolve my confusion. :) Thanks, Serguei On 2/23/20 20:21, Daniil Titov wrote: > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > >// delegate to the actual SA debug server. > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > but I would prefer to address it in a separate issue. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > Thank you, > Daniil > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei, I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid using the system properties and prefer the command line options instead. Thanks, Daniil On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, I've looked at CSR and posted a couple of questions there. It'd be nice if you help to resolve my confusion. :) Thanks, Serguei On 2/23/20 20:21, Daniil Titov wrote: > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > >// delegate to the actual SA debug server. > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > but I would prefer to address it in a separate issue. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > Thank you, > Daniil > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil, I've looked at CSR and posted a couple of questions there. It'd be nice if you help to resolve my confusion. :) Thanks, Serguei On 2/23/20 20:21, Daniil Titov wrote: Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). // delegate to the actual SA debug server. 367 DebugServer.main(newArgArray.toArray(new String[0])); However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated but I would prefer to address it in a separate issue. Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 Thank you, Daniil
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Daniil, - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments. Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply. - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used. But you can use same port number as RMI registry (1099). It is same as relation between jmxremote.port and jmxremote.rmi.port. Thanks, Yasumasa On 2020/02/24 13:21, Daniil Titov wrote: Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. Currently a random port is used that prevents the debug server 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 jhsdb will be updated in a separate issue. The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). // delegate to the actual SA debug server. 367 DebugServer.main(newArgArray.toArray(new String[0])); However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated but I would prefer to address it in a separate issue. Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 Thank you, Daniil
