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

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

Thanks for updating the patch [~gtCarrera9].

I went over the latest patch (v.2), and here is my input:

(TimelineEntity.java)
- l.109: Nit: actually {{obj instanceof Identifier}} returns false if {{obj}} 
is {{null}}. Therefore, you can safely omit the {{obj == null}} check. The same 
goes for the other classes.
- l.533: Shouldn't we check for null from {{getIdentifier()}}? We cannot 
guarantee that it will be called only by callers who checked {{isValid()}}
- l.545: same here
- l.550: It sounds like now the type takes precedence over the created time in 
the sort order in this version. Is this intended? If not (timestamp is supposed 
to be first), it might be a good idea to have {{Identifier}} implement 
{{Comparable}} as well and use that in {{TimelineEntity.compareTo()}}.

(TimelineMetric.java)
- l.149-155: it would perform a little faster to check the id first and then 
the type

> 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, 
> YARN-3836-YARN-2928.002.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