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

Thu Jan 24 01:13:15 UTC 2019

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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
> 