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

[Yasumasa Suenaga]

PING: Could you review it?

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


Yasumasa


On 2019/04/24 21:43, Yasumasa Suenaga wrote:
> 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
>>>>>>
>>