hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sangjin Lee (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-3836) add equals and hashCode to TimelineEntity and other classes in the data model
Date Thu, 09 Jul 2015 01:24:05 GMT

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

Mime
View raw message