[8u] RFR: 8059038: Create new launcher for SA tools

[Andrew Hughes]

On Wed, 12 Dec 2018 at 17:28, Severin Gehwolf <sgehwolf at redhat.com> wrote:
>
> On Wed, 2018-12-12 at 16:18 +0000, Andrew Hughes wrote:
> > On Wed, 12 Dec 2018 at 14:10, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > Can I get a review of this small 8u enhancement, please? It adds two
> > > new launchers for the serviceability agent, one CLI version and one GUI
> > > version:
> > >
> > > $ <image>/bin/clhsdb
> > > $ <image>/bin/hsdb
> > >
> > > The enhancement request has been approved here:
> > > http://mail.openjdk.java.net/pipermail/jdk8u-dev/2018-December/008257.html
> > >
> > > During that discussion it has been suggested to use separate launchers
> > > for GUI and CLI. So this is the revised two-launcher-approach:
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8059038/02/
> > > bug: https://bugs.openjdk.java.net/browse/JDK-8059038
> > >
> > > Note: The initial version of this patch[1] had one launcher "jhsdb"
> > > with sub-commands "hsdb" and "clhsdb" delegating to the old launcher
> > > classes.
> > >
> > > The patch has two simple tests verifying that the launchers work.
> > > Thoughts?
> > >
> > > Thanks,
> > > Severin
> > >
> > > [1] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8059038/01/
> > >
> >
> > The patch itself looks ok. But it looks completely different from the
> > changesets in 9:
> >
> > https://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/bf17c0a1c746
> > https://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/24a8cbde76d8
> >
> > Can you explain a little what's going on here?
>
> Sure. The original patch for 9 added a new class, SALauncher, which
> launches many of the original stand-alone commands from 8. jstack from
> JDK 8, for example, became "jhsdb jstack" in 9+ via this class[I].
>
> On the JDK 8 enhancement request approval thread a suggestion was made
> for the backport to JDK 8 to mimic the stand-alone launcher approach
> JDK 8 uses: "jstack" over "jhsdb jstack". So "clhsdb" (JDK 8) will be a
> separate launcher over "jhsdb clhsdb" in JDK 9+.
>
> The first webrev[1] above, mimic'ed JDK 9+, but not quite since it
> didn't change the arguments and options. jhsdb clhsdb --pid <pid> (JDK
> 9+) vs. jhsdb clhsdb <pid> (in [1]).
>
> That's basically the reason why the JDK 8 backport looks this
> different. Of course we can also go with the original version[1], but
> so far I've heard two +1's for the second version... Hence, I'm
> proposing that version.
>
> Thanks,
> Severin
>
> [I] https://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/24a8cbde76d8#l2.213
>

Ok, I presume that's not a change you're also going to make in OpenJDK 12?

I'm fine with them being linked by the same bug ID, as they resolve the same
underlying issue, but this really needs to be clear in the summary
text, as, to the
uninformed, they look like completely different patches.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222