Hadoop Common
  1. Hadoop Common
  2. HADOOP-5124

A few optimizations to FsNamesystem#RecentInvalidateSets

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.0, 0.21.0
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      This jira proposes a few optimization to FsNamesystem#RecentInvalidateSets:
      1. when removing all replicas of a block, it does not traverse all nodes in the map. Instead it traverse only the nodes that the block is located.
      2. When dispatching blocks to datanodes in ReplicationMonitor. It randomly chooses a predefined number of datanodes and dispatches blocks to those datanodes. This strategy provides fairness to all datanodes. The current strategy always starts from the first datanode.

      1. HADOOP-5124-branch-0.20-security.patch
        8 kB
        Jitendra Nath Pandey
      2. optimizeInvalidate.patch
        4 kB
        Hairong Kuang
      3. optimizeInvalidate1.patch
        8 kB
        Hairong Kuang
      4. optimizeInvalidate2.patch
        7 kB
        Hairong Kuang
      5. optimizeInvalidate2-0.20.2.patch
        5 kB
        Jeff Whiting

        Issue Links

          Activity

          Hide
          Hairong Kuang added a comment -

          This patch goes with a unit testcase.

          Show
          Hairong Kuang added a comment - This patch goes with a unit testcase.
          Hide
          Konstantin Shvachko added a comment -
          1. computeInvalidateWork()
            1. You probably want to use Math.min() in computing the value of nodesToProcess
            2. I would rather go with
              ArrayList<String> keyArray = new ArrayList<String>(recentInvalidateSets.keySet());
              than String[] keyArray. You will be able to use Collections.swap() instead of implementing it yourself.
              Ideally it would be better of course to just get a random element from the TreeMap and put it into the array list.
          2. invalidateWorkForOneNode()
                if(it.hasNext())
                  recentInvalidateSets.put(firstNodeId, invalidateSet);
            

            Is a no op in your case, because recentInvalidateSets already contains firstNodeId with exactly invalidateSet as it was modified before in the loop.
            The original variant of this code

                if(!it.hasNext())
                  recentInvalidateSets.remove(nodeId);
            

            makes more sense since we remove the entire node if it does not have invalid blocks anymore.

          3. Could you please run some tests showing how much of optimization we can get with the randomization of data-node selection.
          Show
          Konstantin Shvachko added a comment - computeInvalidateWork() You probably want to use Math.min() in computing the value of nodesToProcess I would rather go with ArrayList<String> keyArray = new ArrayList<String>(recentInvalidateSets.keySet()); than String[] keyArray . You will be able to use Collections.swap() instead of implementing it yourself. Ideally it would be better of course to just get a random element from the TreeMap and put it into the array list. invalidateWorkForOneNode() if (it.hasNext()) recentInvalidateSets.put(firstNodeId, invalidateSet); Is a no op in your case, because recentInvalidateSets already contains firstNodeId with exactly invalidateSet as it was modified before in the loop. The original variant of this code if (!it.hasNext()) recentInvalidateSets.remove(nodeId); makes more sense since we remove the entire node if it does not have invalid blocks anymore. Could you please run some tests showing how much of optimization we can get with the randomization of data-node selection.
          Hide
          Konstantin Shvachko added a comment -

          Sorry about (2). I thought {if(!it.hasNext())}} is the old code. It is the new one, so it is absolutely correct. Please disregard (2).

          Show
          Konstantin Shvachko added a comment - Sorry about (2). I thought {if(!it.hasNext())}} is the old code. It is the new one, so it is absolutely correct. Please disregard (2).
          Hide
          Hairong Kuang added a comment -

          This patch incorporates Konstantin's first comment.

          As for (3), I am not clear how to evaluate the optimization. The goal of the new strategy is not to improve performance. Instead it aims to give fairness to all nodes when computing invalidation work. The current strategy always favors the ones in the beginning of the map since recentInvalidateSets is a TreeMap so it is sorted. Another flaw is that after the first node is scheduled, the node is reinserted into the map if it still has remaining blocks. Since it becomes the first node again, next call of invalidateWorkForOneNode will work on the same node again. The current strategy would work fine if recentInvalidateSets is a FIFO queue.

          Show
          Hairong Kuang added a comment - This patch incorporates Konstantin's first comment. As for (3), I am not clear how to evaluate the optimization. The goal of the new strategy is not to improve performance. Instead it aims to give fairness to all nodes when computing invalidation work. The current strategy always favors the ones in the beginning of the map since recentInvalidateSets is a TreeMap so it is sorted. Another flaw is that after the first node is scheduled, the node is reinserted into the map if it still has remaining blocks. Since it becomes the first node again, next call of invalidateWorkForOneNode will work on the same node again. The current strategy would work fine if recentInvalidateSets is a FIFO queue.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12399320/optimizeInvalidate2.patch
          against trunk revision 740237.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12399320/optimizeInvalidate2.patch against trunk revision 740237. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3792/console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good to me

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good to me
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I committed this. Thanks, Hairong!

          Show
          Tsz Wo Nicholas Sze added a comment - I committed this. Thanks, Hairong!
          Hide
          Jeff Whiting added a comment -

          I needed this in the 0.20.2 branch and created a patch for it. I thought it might be useful for others.

          Show
          Jeff Whiting added a comment - I needed this in the 0.20.2 branch and created a patch for it. I thought it might be useful for others.
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for branch-0.20-security.

          Show
          Jitendra Nath Pandey added a comment - Patch for branch-0.20-security.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 the 0.20s patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 the 0.20s patch looks good.

            People

            • Assignee:
              Hairong Kuang
              Reporter:
              Hairong Kuang
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development