[ 
https://issues.apache.org/jira/browse/YARN-3836?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14619702#comment-14619702
 ] 

Sangjin Lee commented on YARN-3836:
-----------------------------------

Thanks [~gtCarrera9] for your quick patch! I agree mostly with your 2 points 
above.

I also did take a quick look at the patch, and here are my initial comments.

I see that we're implementing the {{Comparable}} interface for all 3 types. I'm 
wondering if it makes sense for them. What would it mean to order 
{{TimelineEntity}} instances? Does it mean much? Where would it be useful? Do 
we need to implement it? The same questions go for the other 2 types...

(TimelineEntity.java)
What I would prefer is to override {{equals()}} and {{hashCode()}} for 
{{Identifier}} instead, and have simple {{equals()}} and {{hashCode()}} 
implementations for {{TimelineEntity}} that mostly delegate to {{Identifier}}. 
The rationale is that {{Identifier}} can be useful as keys to collections in 
its own right, and thus should override those methods.

One related question for your use case of putting entities into a map: I notice 
that you're using the {{TimelineEntity}} instances directly as keys to maps. 
Wouldn't it be better to use their {{Identifier}} instances as keys instead? 
{{Identifier}} instances are easier and cheaper to construct and compare.

We still need {{equals()}} and {{hashCode()}} on {{TimelineEntity}} itself 
because they can be used in sets too.

- l.42: We should make {{isValid()}} a proper javadoc hyperlink
- l.510: Although this is probably going to be true for the most part, this 
check is a little bit stronger than I expected. We're essentially saying the 
actual class types of two objects must match precisely. People might extend 
classes further. Since we're checking the entity type and the id, wouldn't it 
be sufficient to check whether the object is an instance of {{TimelineEntity}}?

(TimelineEvent.java)
This is an open question. Is the id alone the identity or does the timestamp 
together form the identity? Do we expect users of {{TimelineEvent}} always be 
able to provide the timestamp? Honestly I'm not 100% sure what the contract is, 
and we probably want to make it explicit (and add it to the javadoc). Thoughts?

- l.100: same comment on the class as above

(TimelineMetric.java)
- l.144: same comment on the class as above

> add equals and hashCode to TimelineEntity and other classes in the data model
> -----------------------------------------------------------------------------
>
>                 Key: YARN-3836
>                 URL: https://issues.apache.org/jira/browse/YARN-3836
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Sangjin Lee
>            Assignee: Li Lu
>         Attachments: YARN-3836-YARN-2928.001.patch
>
>
> Classes in the data model API (e.g. {{TimelineEntity}}, 
> {{TimelineEntity.Identifer}}, etc.) do not override {{equals()}} or 
> {{hashCode()}}. This can cause problems when these objects are used in a 
> collection such as a {{HashSet}}. We should implement these methods wherever 
> appropriate.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to