hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sangjin Lee (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-3706) Generalize native HBase writer for additional tables
Date Sat, 06 Jun 2015 00:10:01 GMT

    [ 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)

Mime
View raw message