Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Thu, 9 Jun 2016 15:43:21 +0000 (UTC)
From: "Varun Saxena (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12973337.1464336903000.40646.1465487001213@Atlassian.JIRA>
In-Reply-To: <JIRA.12973337.1464336903000@Atlassian.JIRA>
References: <JIRA.12973337.1464336903000@Atlassian.JIRA> <JIRA.12973337.1464336903265@arcas>
Subject: [jira] [Comment Edited] (YARN-5170) Eliminate singleton converters
 and static method access
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Thu, 09 Jun 2016 15:43:26 -0000


    [ https://issues.apache.org/jira/browse/YARN-5170?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15322719#comment-15322719 ] 

Varun Saxena edited comment on YARN-5170 at 6/9/16 3:43 PM:
------------------------------------------------------------

The patch looks largely good to me.

Few points.
# Maybe its just me, but annotating the whole class as VisibleForTesting looks a little awkward to me. 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. TestKeyConverters for testing event column name and app id converters. And TestRowKeys can use the public *RowKey classes interfaces to have equivalent tests to current ones, which in turn call *RowKeyConverter classes.
This way we would be able to make the *RowKeyConverter classes as package private and will reduce the possibility of someone using it directly.
# In HBaseTimelineWriterImpl#storeEvents, instead of creating EventColumnNameConverter object, we can just use EventColumnName#getColumnQualifier.
# 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.
{code}
  /**
   * @return a byte array with each components/fields separated by
   *         Separator#VALUES. This leads to an event column name of the form
   *         eventId=timestamp=infokey.
   */
{code}
# Any reason TimelineStorageUtils#readEvents has not been moved like readKeyValuePairs and readRelationship ? I guess just missed.
# createColQualifierPrefix has been moved from TimelineFilterUtils to GenericEntityReader. Should it be protected ? Also should it along with createFiltersFromColumnQualifiers be moved to TimelineEntityReader ? I think they should be. For instance, I was trying to use in YARN-5011. That may not be the final solution but still thats an example that we may want to use these methods elsewhere (other than GenericEntityReader and its subclasses).
# 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.
# We keep AppIdKeyConverter as final static but not other converters. Why specifically keep AppIdKeyConverter this way.


was (Author: varun_saxena):
The patch looks largely good to me.

Few points.
# Maybe its just me, but annotating the whole class as VisibleForTesting looks a little awkward to me. 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. TestKeyConverters for testing event column name and app id converters. And TestRowKeys can use the public *RowKey classes interfaces to have equivalent tests to current ones, which in turn call *RowKeyConverter classes.
This way we would be able to make the *RowKeyConverter classes as package private and will reduce the possibility of someone using it directly.
# We keep AppIdKeyConverter as final static but not other converters. Why specifically keep AppIdKeyConverter this way. This is behaving as a singleton then. 
# In HBaseTimelineWriterImpl#storeEvents, instead of creating EventColumnNameConverter object, we can just use EventColumnName#getColumnQualifier.
# 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.
{code}
  /**
   * @return a byte array with each components/fields separated by
   *         Separator#VALUES. This leads to an event column name of the form
   *         eventId=timestamp=infokey.
   */
{code}
# Any reason TimelineStorageUtils#readEvents has not been moved like readKeyValuePairs and readRelationship ? I guess just missed.
# createColQualifierPrefix has been moved from TimelineFilterUtils to GenericEntityReader. Should it be protected ? Also should it along with createFiltersFromColumnQualifiers be moved to TimelineEntityReader ? I think they should be. For instance, I was trying to use in YARN-5011. That may not be the final solution but still thats an example that we may want to use these methods elsewhere (other than GenericEntityReader and its subclasses).
# 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.

> 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org