[
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. Please refer HDFS-10285 to see more detailed
[discussion|https://issues.apache.org/jira/browse/HDFS-10285?focusedCommentId=16347472&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16347472].
*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.
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.
> [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. Please refer HDFS-10285 to see more detailed
> [discussion|https://issues.apache.org/jira/browse/HDFS-10285?focusedCommentId=16347472&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16347472].
> *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: [email protected]
For additional commands, e-mail: [email protected]
