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

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

I took a look at the latest patch, and it looks pretty good overall. I do have 
a few comments. Also, at a high level, were you able to run some tests using a 
pseudo-distributed cluster to verify that it still works as before? If not, 
it'd be great if you can try that out.

(BaseTable.java)
- l.54: nit: space

(RowKey.java/EntityRowKey.java)
- I'm not 100% sure of the value of the inheritance model here. The getRowKey() 
and the getRowKeyPrefix() methods are not common across the supposed subtypes 
(as the arguments change from table to table). If method contracts are not 
shared among the subtypes, there is little commonality among them. In other 
words, you will not be able to use type {{RowKey<EntityTable>}} in code that 
uses it. You'll always have to use {{EntityRowKey}}. Also, it's not like they 
have to implement common instance methods. Then does it warrant the inheritance 
model? Are you considering adding real inherited (instance) methods later?

(Separator.java)
- l.232: Although this gives you a nice way of combining both methods, I'm 
thinking it is OK to provide a separate implementation for the array argument. 
How often can this method be invoked? If it can be invoked often, it may cause 
List's to be created unnecessarily

(TimelineEntitySchemaConstants.java)
- l.62: nit: spacing

> 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-3706-YARN-2928.010.patch, YARN-3706-YARN-2928.011.patch, 
> YARN-3706-YARN-2928.012.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
(v6.3.4#6332)

Reply via email to