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