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.spit...@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@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
|
