[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2020-03-26 13:56:22
Message-ID: BBD463F2-DB3B-4E99-90B2-E46900956E54 () oracle ! com
[Download RAW message or body]
Hi Yasumasa and Serguei,
Thank you for reviewing this change.
Best regards,
--Daniil
On 3/25/20, 1:01 PM, "serguei.spitsyn@oracle.com" <serguei.spitsyn@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.spitsyn@oracle.com" \
<serguei.spitsyn@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" <suenaga@oss.nttdata.com> \
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" \
<suenaga@oss.nttdata.com> 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" \
<suenaga@oss.nttdata.com> 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" \
<suenaga@oss.nttdata.com> 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 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
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic