PING: RFR: 8220657: JFR.dump does not work when filename is set

[Erik Gahlin]

I have looked further at the issue and while the message to the user was 
incorrect previously ( "Dump failed. No data found in the specified 
interval") it did result in a file with the specified filename, i.e 
"test.jfr"

When "clone.setDestination(this.destination);" is removed, the filename 
passed on command line is ignored, but it does result in a message that 
is consistent with what is happening (a file with an autogenerated name 
is created). It does however change the behavior of 'JFR.dump <pid> 
name=1'. Logic needs to be added to DCmdDump so it uses an existing 
filename if it is set, but changes it if a user is providing an override.

When comes to testing. I think it would be better to write a separate 
test that checks that things are dumped correctly 
when-XX:StartFlightRecording:filename=<filename> is used.

Thanks
Erik

> PING: Do you have any opinion(s) about this fix?
>
>
> Yasumasa
>
>
> On 2019/04/01 21:46, Yasumasa Suenaga wrote:
>> Hi,
>>
>> Do you have concern(s) about this change?
>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
>>
>> As in the email below, I think it does not affect scenarios except 
>> snapshot cloning.
>> Please tell me if you have any opinions about this.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/03/20 10:37, Yasumasa Suenaga wrote:
>>> Hi Erik,
>>>
>>> I think this change does not affect other scenarios.
>>>
>>> :jdk_jfr jtreg tests and submit repo have passed with this change.
>>> newSnapshotClone() is called by PlatformRecording::dump and
>>> DCmdDump::newSnapshot.
>>> Both methods are related to user-requested dump.
>>>
>>> What scenarios do you think? I want to check them.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> 2019年3月19日(火) 23:59 Erik Gahlin <erik.gahlin at oracle.com>:
>>>>
>>>> Yes, but then you would lose the original destination in other 
>>>> scenarios
>>>>
>>>> Maybe I can look at this next week.
>>>>
>>>> Erik
>>>>
>>>>> Hi Erik,
>>>>>
>>>>> Thank you for your reply.
>>>>>
>>>>> How about this changeset?
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
>>>>>
>>>>> I think PlatformRecording#newSnapshotClone() should not dump.
>>>>> So I removed `setDestination()` from this.
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/03/19 22:04, Erik Gahlin wrote:
>>>>>> This looks a bit hackish.
>>>>>>
>>>>>> The real issue seems to be that the method
>>>>>> PlatformRecording#newSnapshotClone incorrectly sets the 
>>>>>> destination in
>>>>>> the clone, which then triggers a dump prematurely (when stop is 
>>>>>> called).
>>>>>>
>>>>>> Purpose of that method is just to take a snapshot of existing 
>>>>>> chunks in
>>>>>> the disk repository (or from memory). and then let 
>>>>>> dumpStopped(path) do
>>>>>> the actual dump (copy out the data from the disk repository).
>>>>>>
>>>>>> To make it work, some other changes would be needed as well, but 
>>>>>> I don't
>>>>>> have time to investigate.
>>>>>>
>>>>>> Erik
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this change.
>>>>>>>
>>>>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8220657
>>>>>>>      webrev: 
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.00/
>>>>>>>
>>>>>>> The user might not get flight record via JFR.dump if JFR is started
>>>>>>> with filename option on target process.
>>>>>>>
>>>>>>> Please see JBS if you want to know how to reproduce.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>