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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Jul 9 22:42:04 UTC 2019


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 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