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

[Yasumasa Suenaga]

Hi Erik,

I uploaded new webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.02/

I kept to remove setDestination() call in PlatformRecording.
In this webrev, original filename value will be set to temporary
snapshot if DCmdDump does not have recording name and filename.

Also I added new testcase. It can test JFR.dump with filename.


Thanks,

Yasumasa


On 2019/04/23 6:48, Erik Gahlin wrote:
> 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
>>>>>
>