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

Thu Jan 31 12:43:54 UTC 2019

Hi Erik,

Could you sponsor this?

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

Regards,
Chihiro

On 2019/01/26 16:37, Chihiro Ito wrote:
> I uploaded changeset file as the following URL. Could you push it, 
> please?
>
> http://cr.openjdk.java.net/~cito/JDK-8216565/JDK-8216565-changeset
>
> Regards,
> Chihiro
>
> On 2019/01/26 10:25, Yasumasa Suenaga wrote:
>> Looks good!
>>
>> Erik, can you sponsor this?
>> I'm not an Oracle employee. (I can sponsor this if I can)
>>
>>
>> Yasumasa
>>
>>
>> On 2019/01/26 1:50, Chihiro Ito wrote:
>>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>