[
https://issues.apache.org/jira/browse/YARN-5170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15323601#comment-15323601
]
Li Lu commented on YARN-5170:
-----------------------------
Thanks for the work [~jrottinghuis]! This is a big one and generally LGTM. I
have two questions:
1. In HBaseTimelineWriterImpl
isApplicationEntity and getApplicationEvent seem to be awkward. Looks more like
something related to TimelineEntity or ApplicationEntity, but not just for
storage? Shall we put them to the best place so that the next time people can
directly find them?
2. ApplicationRowKeyConverter, AppToFlowRowKeyConverter, EntityRowKeyConverter,
FlowActivityRowKeyConverter, ... (*RowKeyConverters): can we add them as
private static nested classes for related classes (such as ApplicationRowKey
and AppToFlowRowKey and so on)? In this way we do not need to expose these
classes and restrict users by comments (which may frequently be overlooked).
Other parts looks fine. Thanks!
> 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,
> YARN-5170-YARN-2928.10.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]
