hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Surendra Singh Lilhore (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-13097) [SPS]: Fix the branch review comments(Part1)
Date Wed, 31 Jan 2018 20:33:00 GMT

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

Surendra Singh Lilhore commented on HDFS-13097:
-----------------------------------------------

Fixing below comments

*Comment-1)*
{quote}BlockManager
 Shouldn’t spsMode be volatile? Although I question why it’s here.
{quote}
[Rakesh's reply] Agreed, will do the changes.

*Comment-2)*
{quote}Adding SPS methods to this class implies an unexpected coupling of the SPS service
to the block manager. Please move them out to prove it’s not tightly coupled.
{quote}
[Rakesh's reply] Agreed. I'm planning to create {{StoragePolicySatisfyManager}} and keep all
the related apis over there.

*Comment-5)*
{quote}DatanodeDescriptor
 Why use a synchronized linked list to offer/poll instead of BlockingQueue?
{quote}
[Rakesh's reply] Agreed, will do the changes.

*Comment-8)*
{quote}DFSUtil
 DFSUtil.removeOverlapBetweenStorageTypes and {{DFSUtil.getSPSWorkMultiplier
 }}. These aren’t generally useful methods so why are they in DFSUtil? Why aren’t they
in the only calling class StoragePolicySatisfier?
{quote}
[Rakesh's reply] Agreed, Will do the changes.

*Comment-11)*
{quote}HdfsServerConstants
 The xattr is called user.hdfs.sps.xattr. Why does the xattr name actually contain the word
“xattr”?
{quote}
[Rakesh's reply] Sure, will remove “xattr” word.

*Comment-12)*
{quote}NameNode
 Super trivial but using the plural pronoun “we” in this exception message is odd. Changing
the value isn’t a joint activity.

For enabling or disabling storage policy satisfier, we must pass either none/internal/external
string value only
{quote}
[Rakesh's reply] oops, sorry for the mistake. Will change it.

 
*Comment-16)*
{quote}FSDirStatAndListOp
Not sure why javadoc was changed to add needLocation. It's already present and now doubled
up.{quote}
[Rakesh'r reply] Agreed, will correct it.

 
*Comment-18)*
{quote}DFS_MOVER_MOVERTHREADS_DEFAULT is 1000 per DN? If the DN is concurrently doing 1000
moves, it's not in a good state, disk io is probably saturated, and this will only make it
much worse. 10 is probably more than sufficient.{quote}
[Rakesh'r reply] Agreed, will make it to smaller value 10.
 

> [SPS]: Fix the branch review comments(Part1)
> --------------------------------------------
>
>                 Key: HDFS-13097
>                 URL: https://issues.apache.org/jira/browse/HDFS-13097
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: namenode
>    Affects Versions: HDFS-10285
>            Reporter: Surendra Singh Lilhore
>            Assignee: Surendra Singh Lilhore
>            Priority: Major
>
> Fix the branch review comment.



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

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


Mime
View raw message