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

[Chihiro Ito]

Hi Erik, Yasumasa

Thank you for your advice. I implemented only checking paths. Because I 
wouldn't like to constraints that two objects are identical.

Could you review this, please? If change looks good, please push to the 
repository.

I have run the changes through test-tier1 test sets and saw no sign of 
regression.

webrev:
http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.02/

JBS:
https://bugs.openjdk.java.net/browse/JDK-8216565

Regards,
Chihiro


On 2019/01/22 22:55, Erik Gahlin wrote:
> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>