[
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: [email protected]
For additional commands, e-mail: [email protected]
