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