Details
-
Sub-task
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
HDFS-10285
-
None
Description
Fix the branch review comment. Please refer HDFS-10285 to see more detailed discussion.
Comment-1)
BlockManager
Shouldn’t spsMode be volatile? Although I question why it’s here.
[Rakesh's reply] Agreed, will do the changes.
Comment-2)
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.
[Rakesh's reply] Agreed. I'm planning to create StoragePolicySatisfyManager and keep all the related apis over there.
Comment-5)
DatanodeDescriptor
Why use a synchronized linked list to offer/poll instead of BlockingQueue?
[Rakesh's reply] Agreed, will do the changes.
Comment-8)
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?
[Rakesh's reply] Agreed, Will do the changes.
Comment-11)
HdfsServerConstants
The xattr is called user.hdfs.sps.xattr. Why does the xattr name actually contain the word “xattr”?
[Rakesh's reply] Sure, will remove “xattr” word.
Comment-12)
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
[Rakesh's reply] oops, sorry for the mistake. Will change it.
Comment-15)
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.
[Rakesh'r reply] Agreed, will do the changes.
Comment-16)
FSDirStatAndListOp
Not sure why javadoc was changed to add needLocation. It's already present and now doubled up.
[Rakesh'r reply] Agreed, will correct it.
Comment-18)
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.
[Rakesh'r reply] Agreed, will make it to smaller value 10.
Comment-22)
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.
[Rakesh'r reply] Agreed, will do the changes.