hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joep Rottinghuis (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-5170) Eliminate singleton converters and static method access
Date Thu, 09 Jun 2016 18:37:21 GMT

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

Joep Rottinghuis edited comment on YARN-5170 at 6/9/16 6:37 PM:
----------------------------------------------------------------

[~varun_saxena] Thank you for your review, this is great feedback, I'll work on this and upload
a new patch asap.



was (Author: jrottinghuis):
[~varun_saxena] Thank you for your review, this is great feedback, I'll work on this and upload
a new patch asap.

bq. "The reason the *RowKeyConverter classes need to be public instead of package private
is because of tests in TestKeyConverters. Maybe we can split the TestKeyConverters tests into
TestRowKeys and TestKeyConverters."
Yes, we'll need a test in o.a.h.y.s.t.s.application, one in o.a.h.y.s.t.s.apptoflow, o.a.h.y.s.t.s.entity,
o.a.h.y.s.t.s.flow
It's a little work, but you're right that it is worth it for a cleaner API.

b.q. "The javadoc for EventColumnName#getColumnQualifier below says that event column name
returned from this method will be in the form eventId=timestamp=infokey. But this can return
prefixes too. This will return eventId= if timestamp is null and eventId=timestamp= if infokey
is null."
Indeed this is true. Technically people can just create a rowkey with nulls and still get
partial rowkeys back, in fact that is what the rowkeyprefix does internally. I was thinking
of a way to best describe that #getRowKey was intended for use with complete keys and the
*RowKeyPrefix classes are meant for the prefix ones. This is why I added an interface, which
wasn't strictly needed. I'm wondering what is best, to just describe the intended use of this
method and refer to the prefix classes and methods as a preferred way and leave it like that?
I think if we describe that the getRowKey does return partials in the javadoc I think it can
be considered part of the API and we cannot change that implementation anymore. I can imagine
later having some validation. I'll think about this some more.


> Eliminate singleton converters and static method access
> -------------------------------------------------------
>
>                 Key: YARN-5170
>                 URL: https://issues.apache.org/jira/browse/YARN-5170
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Joep Rottinghuis
>            Assignee: Joep Rottinghuis
>              Labels: yarn-2928-1st-milestone
>         Attachments: YARN-5170-YARN-2928.01.patch, YARN-5170-YARN-2928.02.patch, YARN-5170-YARN-2928.03.patch,
YARN-5170-YARN-2928.04.patch, YARN-5170-YARN-2928.05.patch, YARN-5170-YARN-2928.06.patch,
YARN-5170-YARN-2928.07.patch, YARN-5170-YARN-2928.08.patch, YARN-5170-YARN-2928.09.patch
>
>
> As part of YARN-5109 we introduced several KeyConverter classes.
> To stay consistent with the existing LongConverter in the sample patch I created I made
these other converter classes singleton as well.
> In conversation with [~sjlee0] who has a general dislike of singletons, we discussed
it is best to get rid of these singletons and make them simply instance variables.
> There are other classes where the keys have static methods referring to a singleton converter.
> Moreover, it turns out that due to code evolution we end up creating the same keys several
times.
> So general approach is to not re-instantiate rowkeys, converters when not needed.
> I would like to create the byte[] rowKey in the RowKey classes their constructor, but
that would leak an incomplete object to the converter.
> There are a few method in TimelineStorageUtils that are used only once, or only by one
class, as part of this refactor I'll move these to keep the "Utils" class as small as possible
and keep them for truly generally used utils that don't really belong anywhere else.



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

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message