RFR: 8223066: "jfr metadata" output the @Name annotation twice.

[Chihiro Ito]

Hi Erik,

I removed @Name from the annotations.
Could you review it?

webrev:
http://cr.openjdk.java.net/~cito/JDK-8223066/webrev.01/ 
<http://cr.openjdk.java.net/%7Ecito/JDK-8223066/webrev.01/>

JBS:
https://bugs.openjdk.java.net/browse/JDK-8223066


The output look like this.:

@Name("jdk.ActiveSetting")
@Label("Recording Setting")
@Category("Flight Recorder")
class ActiveSetting extends jdk.jfr.Event {


Regards,
Chihiro

On 2019/05/04 2:48, Chihiro Ito wrote:
> Hi Erik,
>
> Thank you for sharing the background.
>
> In the former case, I feel that PrettyWriter strongly depends on the 
> name and annotation information of Type.
> So I think the latter is better.
>
> Regard
> Chihiro
>
> On 2019/05/02 4:14, Erik Gahlin wrote:
>> Hi Chihiro,
>>
>> Thanks for reporting the issue, @Name should not be printed twice.
>>
>> Let me give you some background information. I think there are two 
>> issues at hand:
>>
>> 1) The Name annotation should probably not be persisted. This can be 
>> achieved by removing the MetadataDefinition from the class. I did try 
>> that, but I noticed several tests failing, in particular TestName 
>> which explicitly checks that @Name is persisted. The purpose @Name is 
>> to override the default name, which can then be retrieved from 
>> ValueDescriptor::getName, EventType::getName and 
>> SettingDescriptor::getName. The annotation is not needed on the 
>> consumer side as the name is already persisted elsewhere.
>>
>> I remember discussing this several times during the design, but for 
>> some reason we came to the conclusion to persist it, probably to not 
>> confuse users if it didn't behave like @Label and @Description. 
>> Later,  Metadatadefinition was removed from @Threshold, @Periodic, 
>> @Enabled and @StackTrace. At that time, it should probably have been 
>> removed from @Name as well. Maybe we should go ahead and remove it 
>> now, but I will have to think about the implications first.
>>
>> 2) The reason PrettyWriter::printType adds @Name is because there is 
>> no good way to include package/namespace in the printout. Events that 
>> lack a namespace should not get the annotation, that's why there is a 
>> check for "." It's not because the values are primitives.
>>
>> I can see two ways to proceed. Either add a check in printType so 
>> @Name is only printed if it doesn't already exists, i.e.
>>
>> if (t.getAnnotation(Name.class) == null) {
>>   if (index != -1) {
>>     println("@Name(\"" + typeName + "\")");
>>   }
>> }
>>
>>  or remove @MetadataDefinition from the Name class and make 
>> adjustments to tests etc. If you chose the latter, I like a week to 
>> think it over, investigate the implications and discuss it with other 
>> people, before I can say if it is the right approach or not.
>>
>> Thanks
>> Erik
>>
>> Hi,
>>>
>>> I fixed an issue that the @Name annotation was printed twice as 
>>> followings in "jfr metadata" output.
>>>
>>> @Name("jdk.ActiveSetting")
>>> @Name("jdk.ActiveSetting")
>>> @Label("Recording Setting")
>>> @Category("Flight Recorder")
>>> class ActiveSetting extends jdk.jfr.Event {
>>>
>>>
>>> Could you reveiew this please?
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~cito/JDK-8223066/webrev.00/ 
>>> <http://cr.openjdk.java.net/%7Ecito/JDK-8223066/webrev.00/>
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8223066
>>>
>>> I have passed all the tests in test-tier1.
>>>
>>> Best regards,
>>> Chihiro
>>
>>
>
>