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" <suen...@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
>
>
