RFR: 8137164: Include PID in the JFR jcmd hint

[Thomas Stüfe]

Hi Erik,

On Thu, Jun 28, 2018 at 9:05 AM, Erik Gahlin <erik.gahlin at oracle.com> wrote:
> Hi Thomas,
>
>> On 28 Jun 2018, at 07:38, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>> Hi Erik,
>>
>> this looks fine to me.
>>
>> Since you are changing the output, could we change filename=FILEPATH
>> to filename=<FILEPATH> or filename=<filepath>, which seems to be the
>> default for "insert name here”?
>
> I don’t remember the rationale for FILEPATH. I will look into it.
>
> Perhaps it was because a shorter placeholder is more likely to fit 80 character.  That was at least the reason the PID was not included originally, but I think the usability aspects of having it printed outweighs such concerns.
>
> It could also be because of how jcmd <pid> help JFR.start references parameters in the help text.
>
> Regardless, I would prefer to handle that as a separate issue.
>

Sure. As I wrote above, your patch is fine to me as it is, with my
limited JFR knowledge.

>>
>> Side note, I was looking at this code closely for the first time and I
>> am surprised how much parsing etc is done on the client side (the
>> jcmd) -
>
> If you mean the changes in the review, it all happens server side.

Ah, I see it now. Ok, that question was stupid :)

Thanks, Thomas

>
> Thanks
> Erik
>
>> I always thought the principle was to leave jcmd dumb and thus
>> compatible and let the hotspot itself do all the work? Sorry if the
>> question is stupid, I do not know much about JFR.
>>
>> Thanks, Thomas
>>
>>
>> On Wed, Jun 27, 2018 at 10:09 PM, Erik Gahlin <erik.gahlin at oracle.com> wrote:
>>> Hi,
>>>
>>> Could I have review of the following fix that adds the PID to the output
>>> when starting a JFR recording.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~egahlin/8137164/
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8137164
>>>
>>> Testing:
>>> test/jdk/jdk/jfr/*
>>>
>>> Thanks
>>> Erik
>