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