Return-Path: 
 <yarn-issues-return-61061-apmail-hadoop-yarn-issues-archive=hadoop.apache.org@hadoop.apache.org>
X-Original-To: apmail-hadoop-yarn-issues-archive@minotaur.apache.org
Delivered-To: apmail-hadoop-yarn-issues-archive@minotaur.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 8195718889
	for <apmail-hadoop-yarn-issues-archive@minotaur.apache.org>;
 Sat,  6 Jun 2015 00:10:01 +0000 (UTC)
Received: (qmail 23033 invoked by uid 500); 6 Jun 2015 00:10:01 -0000
Delivered-To: apmail-hadoop-yarn-issues-archive@hadoop.apache.org
Received: (qmail 22990 invoked by uid 500); 6 Jun 2015 00:10:01 -0000
Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:yarn-issues-help@hadoop.apache.org>
List-Unsubscribe: <mailto:yarn-issues-unsubscribe@hadoop.apache.org>
List-Post: <mailto:yarn-issues@hadoop.apache.org>
List-Id: <yarn-issues.hadoop.apache.org>
Reply-To: yarn-issues@hadoop.apache.org
Delivered-To: mailing list yarn-issues@hadoop.apache.org
Received: (qmail 22979 invoked by uid 99); 6 Jun 2015 00:10:01 -0000
Received: from arcas.apache.org (HELO arcas.apache.org) (140.211.11.28)
    by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 06 Jun 2015 00:10:01 +0000
Date: Sat, 6 Jun 2015 00:10:01 +0000 (UTC)
From: "Sangjin Lee (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12832177.1432307363000.7909.1433549401130@Atlassian.JIRA>
In-Reply-To: <JIRA.12832177.1432307363000@Atlassian.JIRA>
References: <JIRA.12832177.1432307363000@Atlassian.JIRA>
 <JIRA.12832177.1432307363638@arcas>
Subject: [jira] [Commented] (YARN-3706) Generalize native HBase writer for
 additional tables
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394


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

Sangjin Lee commented on YARN-3706:
-----------------------------------

OK, I finally got around to making one pass. These are high level comments (the stack overflow issue notwithstanding). I generally agree with the approach taken here. This will make future implementation work on this a lot safer and easier with less duplication.

(BaseTable.java)
- l.102: how about requiring subclasses to provide the default table name along with the conf name and then provide the default implementation for getTableName()? For example,

{code}
private final String defaultTableName;

protected BaseTable(String tableNameConfName, String defaultTableName) {
  this.tableNameConfName = tableNameConfName;
  this.defaultTableName = defaultTableName;
}

...

public TableName getTableName(Configuration hbaseConf) {
  return TableName.valueOf(hbaseConf.get(tableNameConfName, defaultTableName));
}
{code}
- l.55: I'm not sure if I understand the rationale of the setTableName() method; it sounds more like a static helper method, but then it's really a trivial helper method; should it even be here?

(BufferedMutatorDelegator.java)
- nit: I would remove all the trivial method comments

(EntityTable.java)
- l.92: should be static
- l.111: just curious, is there a strong reason it has to be a singleton? I generally shun singletons (which also causes bit of a challenge with unit testst).

(ColumnImpl.java)
- It doesn't implement Column? Shouldn't it?
- l.57: it should have TypedBufferedMutator<T> as opposed to TypedBufferedMutator<?>, right?

(TimelineEntitySchemaConstants.java)
- l.67: nit: username_splits -> USERNAME_SPLITS
- findbugs will flag any public constants or methods that return the raw byte[]... See if you can live without them (or make them non-public)

(TimelineWriterUtils.java)
- l.72: do you think it'd be possible to do the separator encoding once and keep reusing it? It's probably not terribly expensive, but if it is in a critical path, its cost may add up


> Generalize native HBase writer for additional tables
> ----------------------------------------------------
>
>                 Key: YARN-3706
>                 URL: https://issues.apache.org/jira/browse/YARN-3706
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Joep Rottinghuis
>            Assignee: Joep Rottinghuis
>            Priority: Minor
>         Attachments: YARN-3706-YARN-2928.001.patch, YARN-3726-YARN-2928.002.patch, YARN-3726-YARN-2928.003.patch, YARN-3726-YARN-2928.004.patch, YARN-3726-YARN-2928.005.patch
>
>
> When reviewing YARN-3411 we noticed that we could change the class hierarchy a little in order to accommodate additional tables easily.
> In order to get ready for benchmark testing we left the original layout in place, as performance would not be impacted by the code hierarchy.
> Here is a separate jira to address the hierarchy.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)