[ https://issues.apache.org/jira/browse/HDFS-12225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16134189#comment-16134189 ]
Rakesh R edited comment on HDFS-12225 at 8/19/17 5:17 PM: ---------------------------------------------------------- Thanks [~surendrasingh]. Few minor comments on the patch. Also, please take care checkstyle warnings. # Could you add wait/notify for the PendingSPSTaskScanner re-check, otw it would occupy CPU cycles unnecessary if there are no items in the queue. Adding sample code snippet, {code} if (rootINodeId == null) { // Waiting for SPS path synchronized(pendingSPSxAttrInode){ pendingSPSxAttrInode.wait(5000); } } else { // ... } {code} {code} public void addInodeToPendingSPSList(long id) { pendingSPSxAttrInode.add(id); // Notify waiting PendingSPSTaskScanner thread about the newly added SPS path. synchronized(pendingSPSxAttrInode){ pendingSPSxAttrInode.notify(); } } {code} # Can you cleanup {{pendingSPSxAttrInode}} list on {{SPS#clearQueues()}}. Also, need to call {{SPS#clearQueues()}} on {{SPS#disable()}} function ? # Can we keep the pendingSPSTaskScanner#interrupt call after {{if (storagePolicySatisfierThread == null)}} check like below: {code} StoragePolicySatisfier.java public synchronized void disable(boolean forceStop) { isRunning = false; if (storagePolicySatisfierThread == null) { return; } if (pendingSPSTaskScanner != null) { pendingSPSTaskScanner.interrupt(); } {code} # Please modify the javadoc of {{#isDir()}} to {code} // Returns true if the tracking path is a directory, false otherwise. {code} # Typo: {{block moved in ACHIVE}} -> {{block moved in ARCHIVE}} was (Author: rakeshr): Thanks [~surendrasingh] for the patch. Adding few minor comments: # Could you add wait/notify for the PendingSPSTaskScanner re-check, otw it would occupy CPU cycles unnecessary if there are no items in the queue. Adding sample code snippet, {code} if (rootINodeId == null) { // Waiting for SPS path synchronized(pendingSPSxAttrInode){ pendingSPSxAttrInode.wait(5000); } } else { // ... } {code} {code} public void addInodeToPendingSPSList(long id) { pendingSPSxAttrInode.add(id); // Notify waiting PendingSPSTaskScanner thread about the newly added SPS path. synchronized(pendingSPSxAttrInode){ pendingSPSxAttrInode.notify(); } } {code} # Can you cleanup {{pendingSPSxAttrInode}} list on {{SPS#clearQueues()}}. Also, need to call {{SPS#clearQueues()}} on {{SPS#disable()}} function ? # Can we keep the pendingSPSTaskScanner#interrupt call after {{if (storagePolicySatisfierThread == null)}} check like below: {code} StoragePolicySatisfier.java public synchronized void disable(boolean forceStop) { isRunning = false; if (storagePolicySatisfierThread == null) { return; } if (pendingSPSTaskScanner != null) { pendingSPSTaskScanner.interrupt(); } {code} # Please modify the javadoc of {{#isDir()}} to {code} // Returns true if the tracking path is a directory, false otherwise. {code} # Typo: {{block moved in ACHIVE}} -> {{block moved in ARCHIVE}} > [SPS]: Optimize extended attributes for tracking SPS movements > -------------------------------------------------------------- > > Key: HDFS-12225 > URL: https://issues.apache.org/jira/browse/HDFS-12225 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: datanode, namenode > Reporter: Uma Maheswara Rao G > Assignee: Surendra Singh Lilhore > Attachments: HDFS-12225-HDFS-10285-01.patch, > HDFS-12225-HDFS-10285-02.patch, HDFS-12225-HDFS-10285-03.patch > > > We have discussed to optimize number extended attributes and asked to report > separate JIRA while implementing [HDFS-11150 | > https://issues.apache.org/jira/browse/HDFS-11150?focusedCommentId=15766127&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15766127] > This is the JIRA to track that work > For the context, comment copied from HDFS-11150 > {quote} > [~yuanbo] wrote : I've tried that before. There is an issue here if we only > mark the directory. When recovering from FsImage, the InodeMap isn't built > up, so we don't know the sub-inode of a given inode, in the end, We cannot > add these inodes to movement queue in FSDirectory#addToInodeMap, any > thoughts?{quote} > {quote} > [~umamaheswararao] wrote: I got what you are saying. Ok for simplicity we can > add for all Inodes now. For this to handle 100%, we may need intermittent > processing, like first we should add them to some intermittentList while > loading fsImage, once fully loaded and when starting active services, we > should process that list and do required stuff. But it would add some > additional complexity may be. Let's do with all file inodes now and we can > revisit later if it is really creating issues. How about you raise a JIRA for > it and think to optimize separately? > {quote} > {quote} > [~andrew.wang] wrote in HDFS-10285 merge time review comment : HDFS-10899 > also the cursor of the iterator in the EZ root xattr to track progress and > handle restarts. I wonder if we can do something similar here to avoid having > an xattr-per-file being moved. > {quote} -- This message was sent by Atlassian JIRA (v6.4.14#64029) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org