[ 
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

Reply via email to