[
https://issues.apache.org/jira/browse/YARN-5170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15304653#comment-15304653
]
Joep Rottinghuis commented on YARN-5170:
----------------------------------------
General concern with singletons that they start out innocent w/o state. Then
state creeps in and subtle bug happen.
Main motivation around using singletons for the converters was
a) LongConverter already was singleton, so we kept it consistent
b) Only once instance had to be around and could be re-used
a) will be tackled by refactoring both in this jira
b) Aside from single object creation being cheap (unless we do it so often we
can show it isn't), it turns out that refactoring the static access ends up
with cleaner code in this case. We had cases where we asked a static method to
encode, which would then pull a singleton reference to a converter and create
an instance of the key to pass to it. Some of our other code evolved into
similar patterns.
It turns out that we now keep the converters in instance variables and/or
create them once per call and re-use them still.
We actually re-created rowkeys several times in the call hierarchy of
HBaseTimelineWriterImpl. I'm in the middle of addressing that now.
> 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
> Reporter: Joep Rottinghuis
> Assignee: Joep Rottinghuis
> Attachments: YARN-5170-YARN-2928.01.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]
