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

[Erik Gahlin]

Looks good.

Erik
> Hi Yasumasa,
>
> I changed it to check that the log is contained instead of comparing 
> object ids.
>
> Could you review this again, 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.03/
>
> JBS:
> https://bugs.openjdk.java.net/browse/JDK-8216565
>
> Regards,
> Chihiro
>
> On 2019/01/24 10:13, Yasumasa Suenaga wrote:
>> Hi Chihiro,
>>
>> I don't like testRepository().
>>
>> As I pointed in [1], time resolution for filename is seconds.
>> Is this test valid?
>>
>> ```
>> +            JcmdHelper.jcmd("JFR.configure", REPOSITORYPATH_SETTING_1);
>> +            SafePath initialPath = 
>> Repository.getRepository().getRepositoryPath();
>> +            JcmdHelper.jcmd("JFR.configure", REPOSITORYPATH_SETTING_1);
>> ```
>>
>> If above two jcmd call finish within a second, I think it cannot 
>> check this fix.
>>
>> Your change has log output if same path is passed to JFR.configure.
>> For example, TestJcmdChangeLogLevel checks log output for jfr.
>> You might use same way.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1] 
>> https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2019-January/000360.html
>>
>>
>> On 2019/01/24 1:59, Chihiro Ito wrote:
>>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>