[ https://issues.apache.org/jira/browse/YARN-5170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15322626#comment-15322626 ]
Varun Saxena commented on YARN-5170: ------------------------------------ The patch looks largely good to me. A couple of points. # Annotating the whole class as VisibleForTesting looks a little awkward to me. > 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 > 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-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org