[ https://issues.apache.org/jira/browse/HDFS-13097?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16354035#comment-16354035 ]
Rakesh R commented on HDFS-13097: --------------------------------- Attached new patch addressing the above comments. Please review it again. Thanks! > [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, HDFS-13097-HDFS-10285.03.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: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org