[
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 10:15 PM:
-----------------------------------------------------------------
[~varun_saxena] Thank you for your review, this is great feedback, I'll work on
this and upload a new patch asap.
bq. We have readMetrics method in TimelineEntityReader. Should other read***
methods be moved there as well ? Who knows in future we add a table which needs
to read only KV pairs and hence does not directly belong as a subclass to
GenericEntityReader.
Moved most methods, but left #readKeyValuePairs(TimelineEntity entity, Result
result,
ColumnPrefix<T> prefix, boolean isConfig) in GenericEntityReader given
the specificity from isConfig.
I started to look to refactor entity.addInfo and entity.addConfig to make them
more similar, but I abandoned that part of the refactor to focus on all the
other things that this patch already does.
I think if we can make it more generic and not config specific, then we can
certainly move this method up the hierarchy.
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.
> 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]
