[
https://issues.apache.org/jira/browse/HDFS-13097?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16353781#comment-16353781
]
Uma Maheswara Rao G commented on HDFS-13097:
--------------------------------------------
Thanks for updating the patch. Sorry for coming late, was in travel. Nice
comments and fixes.
I have few more comments to address here.
1.
{code:java}
blockManager.getSpsManager().getService().init(
new IntraSPSNameNodeContext(this, blockManager,
- blockManager.getSPSService()),
+ blockManager.getSpsManager().getService()),
new IntraSPSNameNodeFileIdCollector(getFSDirectory(),
- blockManager.getSPSService()),
+ blockManager.getSpsManager().getService()),
new IntraSPSNameNodeBlockMoveTaskHandler(blockManager, this), null);
{code}
Would it be good if we move this as well under manager?
2.
{code:java}
boolean isSPSRunning = namesystem.getBlockManager().getSpsManager()
+ .isSatisfierRunning();
{code}
For constant naming should we use one form. SPS or Sps ?
3. Typo: StoragePolictSatisfyManager —> StoragePolicySatisfyManager
4. private SPSPathIds paths;
Do we really need separate class for this now? Since we have Manager
class, why can’t maintain
them inside?
5.
{code:java}
spsService = new StoragePolicySatisfier(conf);{code}
could you please add a comment whether object creation of SPS does really
starts any threads inside?
6.
{code:java}
/**
+ * Enable storage policy satisfier by starting its service.
+ */
+ public void enableExternal() {
{code}
This is confusing. This is not really starting any service. It will just make
necessary changes in NN for supporting External SPS
7. StoragePolictSatisfyManager
This is clearly manages sps modes only inside NN. It will not control outside
processes. I think we could document here how the modes managed in this class?
Basically to make it clear that this class manages modes inside NN.
> [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]
