PING: RFR: 8209790: SA tools not providing option to connect to debug server
Chris Plummer
chris.plummer at oracle.com
Mon Jul 8 23:08:44 UTC 2019
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