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
|
