hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Naganarasimha G R (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-7988) Refactor FSNodeLabelStore code for attributes store support
Date Fri, 02 Mar 2018 01:03:00 GMT

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

Naganarasimha G R edited comment on YARN-7988 at 3/2/18 1:02 AM:
-----------------------------------------------------------------

Thanks for working on the patch [~bibinchundatt]
 May be little write up on the class organization would have been a help here as it involves
lot of generics. And also what gets reused in terms of Attributes

Few comments :

FSNodeStoreLogOp.java
 * ln no 46, Documentation required for understanding a class with generics. Else we will
be lost assuming what they represent
 *  ln no 46, Also instead of W,R & M can we have OUT, IN, MGR ? or even more specific
that OUT extends FSDataOutputStream & IN extends FSDataInputStream. Also are we expecting
anything else over here ?
 *  ln no 46, If this class is specific to labels then we should better have it as FSNodeLabelStoreLogOp?
abstract is only used for factoring here.
 *  ln no 53,54,57,58,59,70,100,139,179 Generics havent been used to the fullest and hence
eclipse shows warnings @ these lines. Some can be easily avoided.
 * .ln no 70, getStoreOp is not used anywhere, is it required?

FileSystemNodeLabelsStore.java
 * ln no 44, AbstractFSNodeStore cant be parameterised here ? I think we can have the specifix
manager we want here

FSNodeLogOpCode.java
 * Do we require this static operation ? cant we make use of enum's values() method directly
? and also given the string name of enum we can get the specified Enum.

FSNodeStoreMirrorOp.java
 *  do we require it to be abstract ? we are not doing any common functionality other than
having factory kind of interface here and also IMO NodeLabelMirrorOp should be class alone
and not embedded here

StoreOp.java
 * i think we require one more interface lile _FileStoreOP<M> extends StoreOp<FSDataOutputStream,
FSDataInputStream, M>_ to make other extending classes simplified

Also we have all the new classes under "org.apache.hadoop.yarn.nodelabels.store" package may
be better to add it under some other package name ? And as well PackageInfo.Java is missing
for the new package.

Also lot of checkstyle issues are there which can be addressed, please have a look once the
approach is finalized.


was (Author: naganarasimha):
Thanks for working on the patch [~bibinchundatt]
May be little write up on the class organization would have been a help here as it involves
lot of generics. And also what gets reused in terms of Attributes

Few comments :

FSNodeStoreLogOp.java
 * ln no 46, Documentation required for understanding a class with generics. Else we will
be lost assuming what they represent
 *  ln no 46, Also instead of W,R & M can we have OUT, IN, MGR ? or even more specific
that OUT extends FSDataOutputStream & IN extends FSDataInputStream. Also are we expecting
anything else over here ?
 *  ln no 46, If this class is specific to labels then we should better have it as FSNodeLabelStoreLogOp?
abstract is only used for factoring here.
 *  ln no 53,54,57,58,59,70,100,139,179 Generics havent been used to the fullest and hence
eclipse shows warnings @ these lines. Some can be easily avoided.
 * .ln no 70, getStoreOp is not used anywhere, is it required?


FileSystemNodeLabelsStore.java
 * ln no 44, AbstractFSNodeStore cant be parameterised here ? I think we can have the specifix
manager we want here

FSNodeLogOpCode.java
 * Do we require this static operation ? cant we make use of enum's values() method directly
? and also given the string name of enum we can get the specified Enum.

FSNodeStoreMirrorOp.java
 *  do we require it to be abstract ? we are not doing any common functionality other than
having factory kind of interface here and also IMO NodeLabelMirrorOp should be class alone
and not embedded here

StoreOp.java
 * i think we require one more interface lile _FileStoreOP<M> extends StoreOp<FSDataOutputStream,
FSDataInputStream, M>_ to make other extending classes simplified


Also we have all the new classes under "org.apache.hadoop.yarn.nodelabels.store" package may
be better to add it under some other package name ? And as well PackageInfo.Java is missing
for the new package

> Refactor FSNodeLabelStore code for attributes store support
> -----------------------------------------------------------
>
>                 Key: YARN-7988
>                 URL: https://issues.apache.org/jira/browse/YARN-7988
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Bibin A Chundatt
>            Assignee: Bibin A Chundatt
>            Priority: Major
>         Attachments: YARN-7988-YARN-3409.002.patch, YARN-7988.001.patch
>
>
> # Abstract out file FileSystemStore operation
> # Define EditLog Operartions  and Mirror operation
> # Support compatibility with old nodelabel store



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message