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

Thu Jan 31 20:28:02 UTC 2019

Yes, I can sponsor.  I will try to push it this week, otherwise the next.

Thanks
Erik

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