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 <serguei.spit...@oracle.com>: > 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, serguei.spit...@oracle.com 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, 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 > >>>>>>>>>>>>>> | > >>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>>> > >>>> > >> > >
