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

[Chihiro Ito]

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