RFR: [JDK-8216565] Specifying the same path creates a new directory in JFR.configure

[Erik Gahlin]

Hi Chihiro,

> Hi Erik and Yasumasa,
>
> I am considering what is the best test for this fix. please give me a 
> advice for following question.
>
> Do we need to verify that two objects are same one ?

I assumed you added the check to prevent regressions.

>
> This test verifies that it will not change when the same path is 
> specified in JFR.configure. As long as the same path is specified, 
> even if the results of Repository.getRepository().getRepositoryPath() 
> are different objects, these objects should show the same path. We 
> will not need to verify that it is the same object.
>
If you run the test on JDK 12, it will change the path (object) even if 
it is the same (from an equals standpoint). So a object check will 
ensure that the problem has been fixed in JDK 13. That said, I could 
live without a test, but if you add one. I don't think we should use 
Thread.sleep(2000) as this will slow down testing.

Erik

> Regards,
> Chihiro
>
>
> On 2019/01/15 22:08, Yasumasa Suenaga wrote:
>> Hi,
>>
>> On 2019/01/15 2:29, Erik Gahlin wrote:
>>> Hi Chihiro,,
>>> It would better (and safer) to just compare the references than 
>>> using System.identityHashCode.
>>
>> Does "safer" means two paths points different path?
>> If so, I think you can add sleep between two JFR.configure calls.
>>
>> The resolution of time in Repository::REPO_DATE_FORMAT is seconds.
>> IMHO you can test this change "safely" with sleep 1 sec or longer.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> Asserts.assertTrue(initialPath == samePath));
>>> …
>>> Asserts.assertFalse(initialPath == changedPath));
>>>
>>> Thanks
>>> Erik
>>>
>>>
>>>> On 13 Jan 2019, at 10:59, Chihiro Ito <chihiro.ito at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Thank you for reviewing.
>>>>
>>>> I fix test case and if-statement. Could you review this again ?
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.01/
>>>>
>>>> JBS:
>>>> https://bugs.openjdk.java.net/browse/JDK-8216565
>>>>
>>>> Regards,
>>>> Chihiro
>>>>
>>>> On 2019/01/12 11:05, Yasumasa Suenaga wrote:
>>>>> Hi Chihiro,
>>>>>
>>>>> The condition in if-statement is a little verbosely.
>>>>> I think it becomes more simple if you implement SafePath::equals 
>>>>> and use it.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>> On 2019/01/12 2:11, Chihiro Ito wrote:
>>>>> Hi Erik,
>>>>>
>>>>> Thank you for your review. I didn't have this idea.
>>>>>
>>>>> I'll fix it soon and request a review again.
>>>>>
>>>>> Regards,
>>>>> Chihiro
>>>>>
>>>>> On 2019/01/12 1:24, Erik Gahlin wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Code looks good, but the 2 s sleep in the test unfortunate.
>>>>>>
>>>>>> Perhaps you could compare object identity instead? If it hasn't 
>>>>>> change, it's fine
>>>>>>
>>>>>> Erik
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I reported a following behabior as a bug. [JDK-8216565]
>>>>>>>
>>>>>>> Could I have a reviews for small patch, please ?
>>>>>>>
>>>>>>> -- 
>>>>>>> Even if you specify the same path as repositorypath in 
>>>>>>> JFR.configure, a new subdirectory is created every time.
>>>>>>>
>>>>>>> $ ${JAVA_HOME}/bin/jcmd 17535 JFR.configure repositorypath=/tmp
>>>>>>> 17535:
>>>>>>> Repository path: /tmp/2019_01_12_00_28_53_17535
>>>>>>>
>>>>>>> $ ${JAVA_HOME}/bin/jcmd 17535 JFR.configure repositorypath=/tmp
>>>>>>> 17535:
>>>>>>> Repository path: /tmp/2019_01_12_00_29_03_17535
>>>>>>>
>>>>>>> If the same directory is specified, you should not create a new .
>>>>>>> -- 
>>>>>>>
>>>>>>> Regards,
>>>>>>> Chihiro
>>>>>>>
>>>>>>>
>>>>>>> On 2019/01/10 1:12, Chihiro Ito wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Could I have a reviews for this fix of "jcmd JFR.configure", 
>>>>>>>> please ?
>>>>>>>> This patch passed all tests in test-tier1.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cito/webrev.00/
>>>>>>>>
>>>>>>>> In current implementation even if user specify the same 
>>>>>>>> repository base path in jcmd JFR.configure, hotspot create a 
>>>>>>>> new directory (base path + timestamp) as repository.
>>>>>>>>
>>>>>>>> This patch is hotspot don't create new directory and hotspot 
>>>>>>>> keep to use current one.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Chihiro
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>