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