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 Mon, 08 Jun 2015 17:48:02 GMT

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

Sangjin Lee commented on YARN-3706:

Thanks for the update [~jrottinghuis]! I haven't gone over the latest patch, but will do so
some time soon. Just to reply to your questions earlier...

Reason I had the relatively simple BaseTable.setTableName(...) is that it allows me to not
have to "leak" the name of the configuration value for the table to be a public attribute.
Do you think it is better to just have a public member on EntityTable and set the value directly,
or to keep that private?

Understood. That said, though, configuration names and the default values seem like pretty
public attributes, given we allow people to set and override them. Is it critical we hide
these strings from code? I suspect not...

Wrt. EntityTable.java
I92 should be static. Not sure what you mean by this.

In v.5 I reviewed, DEFAULT_ENTITY_NAME_BYTES was not static. It looks like it is no longer
there in the latest version?

bq. Would you prefer if getInstance simple news up a table instance each time, or are you
generally against the pattern of being able to call getInstance()?

It's really about the singleton pattern. To be sure, this is a fairly minor design bias of
mine. I believe it is normally rather uncommon that something truly must be a singleton or
the code becomes incorrect. Also, the singleton becomes bit of an anti-pattern with regards
to dependency injection and can cause some issues with writing unit tests.

I agree with the USERNAME_SPLITS being public. I've left a comment to remove this completely
and have this read from the configuration. I think it would be better to provide a default
property in a config file for this. This was in place in the code currently checked in and
I did not tackle that in this patch. Would it be OK if I file a separate jira for this?

It was really a small nit about the hadoop coding convention. All static final variables (constants),
whether they are public or not, must be in upper case.

> 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, YARN-3726-YARN-2928.006.patch,
YARN-3726-YARN-2928.007.patch, YARN-3726-YARN-2928.008.patch, YARN-3726-YARN-2928.009.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

View raw message