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 <[email protected]
<mailto:[email protected]>>:
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, [email protected]
<mailto:[email protected]> 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, [email protected]
<mailto:[email protected]> 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@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@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
>>>>>>>>>>>>>> |
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
