PING: RFR: 8209790: SA tools not providing option to connect to debug server

Yasumasa Suenaga yasuenag at gmail.com
Wed Jul 10 23:15:17 UTC 2019


Thanks Serguei!

Yasumasa


On 2019/07/11 2:55, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
> 
> Thank you for adding your comment to the CSR!
> It looks good to me.
> 
> Thanks,
> Serguei
> 
> 
> On 7/9/19 18:20, Yasumasa Suenaga wrote:
>> Hi Serguei,
>>
>> I added a comment that the change from the latest (approved) proposal to the CSR. Could you check it again?
>>
>> Please tell me if it is not enough.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>> 2019年7月10日(水) 8:12 <serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com>>:
>>
>>     Hi Yasumasa,
>>
>>     I'd like you to be more precise.
>>     Providing the webrev, email exchange and saying about help messages is
>>     not precise.
>>     It requires some non-trivial extra work.
>>     Could you add a comment with this:
>>       What help message has been changed to what?
>>
>>     Thanks,
>>     Serguei
>>
>>     On 7/9/19 3:51 PM, Yasumasa Suenaga wrote:
>>     > Hi Serguei,
>>     >
>>     > I added a comment to the CSR.
>>     > I've updated help messages in Specification section of the CSR.
>>     >
>>     >
>>     > Thanks,
>>     >
>>     > Yasumasa
>>     >
>>     >
>>     > On 2019/07/10 7:42, serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> wrote:
>>     >> Hi Yasumasa,
>>     >>
>>     >> Could you be more precise?
>>     >> What was the latest change in this CSR?
>>     >> Could you add it as a comment?
>>     >> It will safe time for reviewers and approvers.
>>     >>
>>     >> Thanks,
>>     >> Serguei
>>     >>
>>     >> On 7/9/19 3:35 PM, Yasumasa Suenaga wrote:
>>     >>> Thanks Serguei!
>>     >>>
>>     >>> I reopened the CSR and update the description.
>>     >>> Could you review again it?
>>     >>>
>>     >>> https://bugs.openjdk.java.net/browse/JDK-8224979
>>     >>>
>>     >>>
>>     >>> Thanks,
>>     >>>
>>     >>> Yasumasa
>>     >>>
>>     >>>
>>     >>> On 2019/07/10 4:11, serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> wrote:
>>     >>>> Hi Yasumasa,
>>     >>>>
>>     >>>> The update looks Okay to me.
>>     >>>>
>>     >>>> Thanks,
>>     >>>> Serguei
>>     >>>>
>>     >>>>
>>     >>>> On 7/8/19 4:16 PM, Yasumasa Suenaga wrote:
>>     >>>>> Thank you Chris!
>>     >>>>>
>>     >>>>> Serguei, do you agree with this change?
>>     >>>>>
>>     >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.07/
>>     >>>>>
>>     >>>>> If you are ok, I will reopen the CSR (JDK-8224979).
>>     >>>>>
>>     >>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028624.html
>>     >>>>>
>>     >>>>>
>>     >>>>>
>>     >>>>> Thanks,
>>     >>>>>
>>     >>>>> Yasumasa
>>     >>>>>
>>     >>>>>
>>     >>>>> On 2019/07/09 8:08, Chris Plummer wrote:
>>     >>>>>> Looks good! Sorry about the high number of iterations. I should
>>     >>>>>> have caught some things sooner.
>>     >>>>>>
>>     >>>>>> thanks,
>>     >>>>>>
>>     >>>>>> Chris
>>     >>>>>>
>>     >>>>>> On 7/8/19 3:43 PM, Yasumasa Suenaga wrote:
>>     >>>>>>> Hi Chris,
>>     >>>>>>>
>>     >>>>>>> Thank you for your comment.
>>     >>>>>>> I fixed it in new webrev:
>>     >>>>>>>
>>     >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.07/
>>     >>>>>>>
>>     >>>>>>> Could you check again?
>>     >>>>>>>
>>     >>>>>>>
>>     >>>>>>> Yasumasa
>>     >>>>>>>
>>     >>>>>>>
>>     >>>>>>> On 2019/07/09 4:11, Chris Plummer wrote:
>>     >>>>>>>> Hi Yasumasa,
>>     >>>>>>>>
>>     >>>>>>>> Sorry, just noticed one other thing:
>>     >>>>>>>>
>>     >>>>>>>>>     <--core --exe> and <--pid> and <--connect> are mutually
>>     >>>>>>>>> exclusive.
>>     >>>>>>>> I'm not sure why you are using angled brackets here. It's not
>>     >>>>>>>> done in other parts of the help output that reference options
>>     >>>>>>>> by name. Also, no need to mention --exe since there is an
>>     >>>>>>>> earlier comment in the help saying in must be used together
>>     >>>>>>>> with --core. This should help to simplify the help message. I
>>     >>>>>>>> would suggest:
>>     >>>>>>>>
>>     >>>>>>>>      --core, --pid, and --connect are mutually exclusive.
>>     >>>>>>>>
>>     >>>>>>>> There's also a typo in the version of the help that does not
>>     >>>>>>>> include --connect:
>>     >>>>>>>>
>>     >>>>>>>>    76 System.out.println(" <--core --exe> and <--pid> are
>>     >>>>>>>> mutually eexclusive.");
>>     >>>>>>>>
>>     >>>>>>>> Notice "eexclusive".
>>     >>>>>>>>
>>     >>>>>>>> Otherwise it looks good.
>>     >>>>>>>>
>>     >>>>>>>> thanks,
>>     >>>>>>>>
>>     >>>>>>>> Chris
>>     >>>>>>>>
>>     >>>>>>>> On 7/8/19 5:06 AM, Yasumasa Suenaga wrote:
>>     >>>>>>>>> Hi Chris,
>>     >>>>>>>>>
>>     >>>>>>>>> I uploaded new webrev:
>>     >>>>>>>>>
>>     >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.06/
>>     >>>>>>>>>
>>     >>>>>>>>> It shows help messages as below:
>>     >>>>>>>>>
>>     >>>>>>>>> ```
>>     >>>>>>>>> $ jhsdb jstack --help
>>     >>>>>>>>> --locks                 To print java.util.concurrent locks.
>>     >>>>>>>>> --mixed                 To print both Java and native
>>     >>>>>>>>> frames (mixed mode).
>>     >>>>>>>>>     --pid <pid>             To attach to and operate on the
>>     >>>>>>>>> given live process.
>>     >>>>>>>>>     --core <corefile>       To operate on the given core file.
>>     >>>>>>>>>     --exe <executable for corefile>
>>     >>>>>>>>>     --connect [<id>@]<host> To connect to a remote debug
>>     >>>>>>>>> server (debugd).
>>     >>>>>>>>>
>>     >>>>>>>>>     The --core and --exe options must be set together to give
>>     >>>>>>>>> the core
>>     >>>>>>>>>     file, and associated executable, to operate on. They can use
>>     >>>>>>>>>     absolute or relative paths.
>>     >>>>>>>>>     The --pid option can be set to operate on a live process.
>>     >>>>>>>>>     The --connect option can be set to connect to a debug
>>     >>>>>>>>> server (debugd).
>>     >>>>>>>>>     <--core --exe> and <--pid> and <--connect> are mutually
>>     >>>>>>>>> exclusive.
>>     >>>>>>>>>
>>     >>>>>>>>>     Examples: jhsdb jstack --pid 1234
>>     >>>>>>>>>           or  jhsdb jstack --core ./core.1234 --exe ./myexe
>>     >>>>>>>>>           or  jhsdb jstack --connect debugserver
>>     >>>>>>>>>           or  jhsdb jstack --connect id at debugserver
>>     >>>>>>>>> ```
>>     >>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> Thanks,
>>     >>>>>>>>>
>>     >>>>>>>>> Yasumasa
>>     >>>>>>>>>
>>     >>>>>>>>>
>>     >>>>>>>>> On 2019/07/08 16:14, Chris Plummer wrote:
>>     >>>>>>>>>> Sorry, I meant "jhsdb debugd", not "clhsdb debugd".
>>     >>>>>>>>>>
>>     >>>>>>>>>> Chris
>>     >>>>>>>>>>
>>     >>>>>>>>>> On 7/8/19 12:09 AM, Chris Plummer wrote:
>>     >>>>>>>>>>> Hi Yasumasa,
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> I just noticed one more thing:
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>   65 System.out.println(" --connect <[id@]host>
>>     >>>>>>>>>>> To connect to a remote debug server (sadebugd).");
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> Earlier on I incorrectly said that jstack and other tools
>>     >>>>>>>>>>> had a references to "sadebugd" in the help output. It is
>>     >>>>>>>>>>> actually "jsadebugd". But that was the name of the command
>>     >>>>>>>>>>> line tool, which is now gone. Instead now we have "clhsdb
>>     >>>>>>>>>>> debugd ...", so maybe it should just mention "debugd".
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> thanks,
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> Chris
>>     >>>>>>>>>>>
>>     >>>>>>>>>>> On 7/7/19 10:24 PM, Chris Plummer wrote:
>>     >>>>>>>>>>>> Hi Yasumasa,
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>> I think it should be "[<id>@]<host>". Angle brackets are
>>     >>>>>>>>>>>> used to indicate non-literal values (ones to be replaced by
>>     >>>>>>>>>>>> something the user chooses), but the '@' is a literal
>>     >>>>>>>>>>>> value, so should not be enclosed in the angle brackets.
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>> Also one minor thing. You ended up duplicating the //:
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>>   60 // // --connect <[id@]host>
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>> thanks,
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>> Chris
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>> On 7/6/19 5:55 PM, Yasumasa Suenaga wrote:
>>     >>>>>>>>>>>>> Hi Chris,
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> Thank you for your advise.
>>     >>>>>>>>>>>>> I uploaded new webrev. How about this?
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.04/
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> I replaced "server" to "host" as you said.
>>     >>>>>>>>>>>>> Other comments from you also have been fixed in it.
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> For example, `jhsdb jstack --help` shows as below:
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> ```
>>     >>>>>>>>>>>>> $ jhsdb jstack --help
>>     >>>>>>>>>>>>> --locks                 To print java.util.concurrent
>>     >>>>>>>>>>>>> locks.
>>     >>>>>>>>>>>>> --mixed                 To print both Java and native
>>     >>>>>>>>>>>>> frames (mixed mode).
>>     >>>>>>>>>>>>>     --pid <pid>             To attach to and operate on
>>     >>>>>>>>>>>>> the given live process.
>>     >>>>>>>>>>>>> --core <corefile>       To operate on the given core
>>     >>>>>>>>>>>>> file.
>>     >>>>>>>>>>>>>     --exe <executable for corefile>
>>     >>>>>>>>>>>>> --connect <[id@]host>   To connect to a remote debug
>>     >>>>>>>>>>>>> server (sadebugd).
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>>     The --core and --exe options must be set together to
>>     >>>>>>>>>>>>> give the core
>>     >>>>>>>>>>>>>     file, and associated executable, to operate on. They
>>     >>>>>>>>>>>>> can use
>>     >>>>>>>>>>>>> absolute or relative paths.
>>     >>>>>>>>>>>>>     The --pid option can be set to operate on a live process.
>>     >>>>>>>>>>>>>     The --connect option can be set to connect to a debug
>>     >>>>>>>>>>>>> server (sadebugd).
>>     >>>>>>>>>>>>> <--core --exe> and <--pid> and <--connect> are exclusive.
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> Examples: jhsdb jstack --pid 1234
>>     >>>>>>>>>>>>> or  jhsdb jstack --core ./core.1234 --exe ./myexe
>>     >>>>>>>>>>>>> or  jhsdb jstack --connect debugserver
>>     >>>>>>>>>>>>> or  jhsdb jstack --connect id at debugserver
>>     >>>>>>>>>>>>> ```
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> If you are ok, I will reopen CSR (JDK-8224979).
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> Yasumasa
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>>
>>     >>>>>>>>>>>>> On 2019/07/06 6:26, Chris Plummer wrote:
>>     >>>>>>>>>>>>>> On 7/4/19 3:53 PM, Yasumasa Suenaga wrote:
>>     >>>>>>>>>>>>>>> Hi Chris,
>>     >>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>> On 2019/07/05 4:24, Chris Plummer wrote:
>>     >>>>>>>>>>>>>>>> Hi Yasumasa,
>>     >>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>> On 7/4/19 5:30 AM, Yasumasa Suenaga wrote:
>>     >>>>>>>>>>>>>>>>> Hi Chris,
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> Thank you for your review.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> On 2019/07/04 8:07, Chris Plummer wrote:
>>     >>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> Overall these changes look good, but I think there is
>>     >>>>>>>>>>>>>>>>>> a bit of cleanup still needed.
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> You've changed the indentation of the help to always
>>     >>>>>>>>>>>>>>>>>> have a few spaces before the /t. If you are going to
>>     >>>>>>>>>>>>>>>>>> do this, then perhaps you don't need the /t anymore.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> Ok, I will replace '\t' to whitespace.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> There are 3 checks for "remote != null". Shouldn't
>>     >>>>>>>>>>>>>>>>>> they be "remote != NO_REMOTE"?
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> I will fix them.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> The old jstack help output is a bit more informative
>>     >>>>>>>>>>>>>>>>>> than your output:
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>>      jstack [-m] [-l] [server_id@]<remote server IP
>>     >>>>>>>>>>>>>>>>>> or hostname>
>>     >>>>>>>>>>>>>>>>>>          (to connect to a remote debug server)
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> vs
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>>      --connect <[id@]server> To operate on the remote
>>     >>>>>>>>>>>>>>>>>> debug server.
>>     >>>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>>> More specifically, you replaced <remote server IP or
>>     >>>>>>>>>>>>>>>>>> hostname> with "server". I think the former is a bit
>>     >>>>>>>>>>>>>>>>>> more self explanatory.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> Exactly, but it is too long to show on the console.
>>     >>>>>>>>>>>>>>>>> So I used short word: server.
>>     >>>>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>>>> If we can ignore long line, or can add newline (\n) on
>>     >>>>>>>>>>>>>>>>> help message,
>>     >>>>>>>>>>>>>>>>> I can change it. What do you think?
>>     >>>>>>>>>>>>>>>> I don't see why not. Isn't there already help output
>>     >>>>>>>>>>>>>>>> that relies on newlines?
>>     >>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>> As you can see on the CSR (JDK-8224979), format of jhsdb
>>     >>>>>>>>>>>>>>> help messages is:
>>     >>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>> <option name> <blank (includes tab)> <description>
>>     >>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>> Thus I think long option / description is not comfortable.
>>     >>>>>>>>>>>>>>> If we need to describe more, I think we should write it
>>     >>>>>>>>>>>>>>> as below:
>>     >>>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>>> --connect <[server-id@]remote server IP or hostname>
>>     >>>>>>>>>>>>>>>       To connect to a remote debug server.
>>     >>>>>>>>>>>>>> Hi Yasumasa,
>>     >>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>> We already have multiline examples like:
>>     >>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>> |--pid <pid> To attach to and operate on the given live
>>     >>>>>>>>>>>>>> process.
>>     >>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>> Is your concerned that the long option name will extend
>>     >>>>>>>>>>>>>> into the columns where the description normally is? If
>>     >>>>>>>>>>>>>> so, you can start the description on a new line as you
>>     >>>>>>>>>>>>>> suggest, or maybe come up with a different name for the
>>     >>>>>>>>>>>>>> option that isn't quite as long. My main objection to
>>     >>>>>>>>>>>>>> just saying "server" is it in no way conveys we are
>>     >>>>>>>>>>>>>> talking about an IP host. Even just saying <host> would
>>     >>>>>>>>>>>>>> be better. It is also more correct since the host isn't
>>     >>>>>>>>>>>>>> necessarily what someone would consider to be a server
>>     >>>>>>>>>>>>>> (it could just be someone's desktop). <server-id> on the
>>     >>>>>>>>>>>>>> other hand is the correct term because we are talking
>>     >>>>>>>>>>>>>> about the id of the debug server running on a particular
>>     >>>>>>>>>>>>>> host.
>>     >>>>>>>>>>>>>>
>>     >>>>>>>>>>>>>> Chris
>>     >>>>>>>>>>>>>> |
>>     >>>>>>>>>>>>>>
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>>
>>     >>>>>>>>>>>
>>     >>>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>>>
>>     >>>>>>>>
>>     >>>>>>>>
>>     >>>>>>
>>     >>>>>>
>>     >>>>
>>     >>
>>
> 


More information about the serviceability-dev mailing list