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] [Updated] (HDFS-13097) [SPS]: Fix the branch review comments(Part1)
Date Tue, 06 Feb 2018 07:14:00 GMT

     [ https://issues.apache.org/jira/browse/HDFS-13097?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Surendra Singh Lilhore updated HDFS-13097:
------------------------------------------
    Description: 
Fix the branch review comment.

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-15)*
{quote}
FSDirSatisfyStoragePolicyOp
satisfyStoragePolicy errors if the xattr is already present. Should this be a no-op? A client
re-requesting a storage policy correction probably shouldn't fail.

unprotectedSatisfyStoragePolicy is called prior to xattr updates, which calls addSPSPathId.
To avoid race conditions or inconsistent state if the xattr fails, should call addSPSPathId
after xattrs are successfully updated.

inodeHasSatisfyXAttr calls getXAttrFeature then immediately shorts out if the inode isn't
a file. Should do file check first to avoid unnecessary computation.

In general, not fond of unnecessarily guava. Instead of newArrayListWithCapacity + add, standard
Arrays.asList(item) is more succinct.
{quote}
[Rakesh'r reply] Agreed, will do the changes.

*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.
 
*Comment-22)*
{quote}
StoragePolicySatisifier
Should handleException use a double-checked lock to avoid synchronization? Unexpected exceptions
should be a rarity, right?
Speaking of which, it’s not safe to ignore all Throwable in the run loop! You have no idea
if data structures are in a sane or consistent state.
{quote}
[Rakesh'r reply] Agreed, will do the changes.

  was:Fix the branch review comment.


> [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
>         Attachments: HDFS-13097-HDFS-10285.01.patch, HDFS-13097-HDFS-10285.02.patch
>
>
> Fix the branch review comment.
> 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-15)*
> {quote}
> FSDirSatisfyStoragePolicyOp
> satisfyStoragePolicy errors if the xattr is already present. Should this be a no-op?
A client re-requesting a storage policy correction probably shouldn't fail.
> unprotectedSatisfyStoragePolicy is called prior to xattr updates, which calls addSPSPathId.
To avoid race conditions or inconsistent state if the xattr fails, should call addSPSPathId
after xattrs are successfully updated.
> inodeHasSatisfyXAttr calls getXAttrFeature then immediately shorts out if the inode isn't
a file. Should do file check first to avoid unnecessary computation.
> In general, not fond of unnecessarily guava. Instead of newArrayListWithCapacity + add,
standard Arrays.asList(item) is more succinct.
> {quote}
> [Rakesh'r reply] Agreed, will do the changes.
> *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.
>  
> *Comment-22)*
> {quote}
> StoragePolicySatisifier
> Should handleException use a double-checked lock to avoid synchronization? Unexpected
exceptions should be a rarity, right?
> Speaking of which, it’s not safe to ignore all Throwable in the run loop! You have
no idea if data structures are in a sane or consistent state.
> {quote}
> [Rakesh'r reply] Agreed, will do the changes.



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