Hadoop Common
  1. Hadoop Common
  2. HADOOP-6433

Add AsyncDiskService that is used in both hdfs and mapreduce

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      HADOOP-6433. Add AsyncDiskService for asynchronous disk services.

      Description

      Both MAPREDUCE-1213 and HDFS-611 are using a class called AsyncDiskService.
      The idea is to create a thread pool per disk volume, and use that for scheduling async disk operations.

      1. HADOOP-6433.1.patch
        7 kB
        Zheng Shao
      2. HADOOP-6433.2.patch
        8 kB
        Zheng Shao
      3. HADOOP-6433.3.patch
        8 kB
        Zheng Shao
      4. HADOOP-6433.3.branch_20.patch
        8 kB
        Zheng Shao

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #188 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/188/)
          . Introduce asychronous deletion of files via a pool of
          threads. This can be used to delete files in the Distributed Cache.
          (Zheng Shao via dhruba)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #188 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/188/ ) . Introduce asychronous deletion of files via a pool of threads. This can be used to delete files in the Distributed Cache. (Zheng Shao via dhruba)
          Hide
          Zheng Shao added a comment -

          This is for hadoop 0.20. May be useful for Y!/Cloudera distribution of Hadoop 0.20.

          Show
          Zheng Shao added a comment - This is for hadoop 0.20. May be useful for Y!/Cloudera distribution of Hadoop 0.20.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #109 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/109/)
          . Introduce asychronous deletion of files via a pool of
          threads. This can be used to delete files in the Distributed Cache.
          (Zheng Shao via dhruba)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #109 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/109/ ) . Introduce asychronous deletion of files via a pool of threads. This can be used to delete files in the Distributed Cache. (Zheng Shao via dhruba)
          Hide
          dhruba borthakur added a comment -

          I just committed this. Thanks Zheng. Please see if you can fill up the release notes in this JIRA.

          Show
          dhruba borthakur added a comment - I just committed this. Thanks Zheng. Please see if you can fill up the release notes in this JIRA.
          Hide
          dhruba borthakur added a comment -

          sounds fine to me.

          Show
          dhruba borthakur added a comment - sounds fine to me.
          Hide
          Zheng Shao added a comment -

          AsyncDiskService is designed to work with multiple disk volumes, to make sure we have a maximum number of threads per volume.
          Both the use cases in HDFS and Mapreduce are like that.

          I don't think it will be used outside this area in the near future - for non-multi-volume disk operations, people will probably use a single thread pool instead of AsyncDiskService (which contains multiple thread pools)

          So I feel it's good to name it AsyncDiskService instead of AsyncService.

          Show
          Zheng Shao added a comment - AsyncDiskService is designed to work with multiple disk volumes, to make sure we have a maximum number of threads per volume. Both the use cases in HDFS and Mapreduce are like that. I don't think it will be used outside this area in the near future - for non-multi-volume disk operations, people will probably use a single thread pool instead of AsyncDiskService (which contains multiple thread pools) So I feel it's good to name it AsyncDiskService instead of AsyncService.
          Hide
          dhruba borthakur added a comment -

          Isn't it the case that the AsyncDiskService has nothing to do with disks per se, but rather a service that allows executing a set of Runnable objects in parallel asynchronously. If my understanding is correct, is it appropriate to name it as AsyncService?

          Show
          dhruba borthakur added a comment - Isn't it the case that the AsyncDiskService has nothing to do with disks per se, but rather a service that allows executing a set of Runnable objects in parallel asynchronously. If my understanding is correct, is it appropriate to name it as AsyncService?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12427780/HADOOP-6433.3.patch
          against trunk revision 889378.

          +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 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-h4.grid.sp2.yahoo.net/199/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/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/12427780/HADOOP-6433.3.patch against trunk revision 889378. +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 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-h4.grid.sp2.yahoo.net/199/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/199/console This message is automatically generated.
          Hide
          Zheng Shao added a comment -

          Addressed all 4 items in Todd's last comment.

          Show
          Zheng Shao added a comment - Addressed all 4 items in Todd's last comment.
          Hide
          Todd Lipcon added a comment -

          Hey Zheng,

          Looks mostly good. Couple nits I missed in the first review (sorry about that!)

          • Comment in ExampleTask says "A task for deleting a pathName", but that's a copy-paste
          • "@throws Throwable" in the javadoc is not necessary
          • In awaitTermination() you could end up calling awaitTermination with a negative argument. I imagine this will probably throw an IllegalArgumentException or something. You should do Math.max(end - System.currentTimeMillis(), 0) I guess.
          • Is warn level logging appropriate for "All AsyncDiskService threads are terminated"? Seems like info, not warn.

          Thanks!

          Show
          Todd Lipcon added a comment - Hey Zheng, Looks mostly good. Couple nits I missed in the first review (sorry about that!) Comment in ExampleTask says "A task for deleting a pathName", but that's a copy-paste "@throws Throwable" in the javadoc is not necessary In awaitTermination() you could end up calling awaitTermination with a negative argument. I imagine this will probably throw an IllegalArgumentException or something. You should do Math.max(end - System.currentTimeMillis(), 0) I guess. Is warn level logging appropriate for "All AsyncDiskService threads are terminated"? Seems like info, not warn. Thanks!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12427683/HADOOP-6433.2.patch
          against trunk revision 889378.

          +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 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-h4.grid.sp2.yahoo.net/195/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/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/12427683/HADOOP-6433.2.patch against trunk revision 889378. +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 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-h4.grid.sp2.yahoo.net/195/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/195/console This message is automatically generated.
          Hide
          Zheng Shao added a comment -

          Can you convert the unit test to JUnit 4 style? (ie using annotations instead of extending TestCase)

          Added "@Test".

          Can you add a test case for a volume that is not in vols?

          Added.

          Rather than sleeping in the test case, can you call shutdown and then awaitTermination() with a longer timeout? This should ensure that the test is allowed to run longer if the machine is super slow, but doesn't waste time if not necessary. This would also make you add an awaitTermination() call, which I think is a good thing to expose.

          Yes. Done.

          What's with the changes to FsShell? Was this accidental?

          Yes. Removed.

          Missing license header on AsyncDIskService.java

          Added.

          The comment still says "In the future, we should put it in common". Since this is a common jira, please remove that

          Removed.

          IMO the thread counts per volume should be configurable. Some systems can handle plenty of concurrent metadata ops while others, not so much. I don't feel strongly about this, though - happy to leave it to a later JIRA if people want this.

          If the system can handle a lot of concurrent ops, that usually means the system is fast, so that we should be able to finish the ops pretty quickly. As a result I think there is not much need to change this option in reality. Let's postpone it until people ask for it.

          Should the threads be made daemon threads? I'm not sure what the expected behavior should be when the server gets SIGINTed, but I think if they're not daemon threads it will block server shutdown.

          Added "shutdownNow()" so that people can force shutdown.

          Show
          Zheng Shao added a comment - Can you convert the unit test to JUnit 4 style? (ie using annotations instead of extending TestCase) Added "@Test". Can you add a test case for a volume that is not in vols? Added. Rather than sleeping in the test case, can you call shutdown and then awaitTermination() with a longer timeout? This should ensure that the test is allowed to run longer if the machine is super slow, but doesn't waste time if not necessary. This would also make you add an awaitTermination() call, which I think is a good thing to expose. Yes. Done. What's with the changes to FsShell? Was this accidental? Yes. Removed. Missing license header on AsyncDIskService.java Added. The comment still says "In the future, we should put it in common". Since this is a common jira, please remove that Removed. IMO the thread counts per volume should be configurable. Some systems can handle plenty of concurrent metadata ops while others, not so much. I don't feel strongly about this, though - happy to leave it to a later JIRA if people want this. If the system can handle a lot of concurrent ops, that usually means the system is fast, so that we should be able to finish the ops pretty quickly. As a result I think there is not much need to change this option in reality. Let's postpone it until people ask for it. Should the threads be made daemon threads? I'm not sure what the expected behavior should be when the server gets SIGINTed, but I think if they're not daemon threads it will block server shutdown. Added "shutdownNow()" so that people can force shutdown.
          Hide
          Todd Lipcon added a comment -

          Hi Zheng,

          • Can you convert the unit test to JUnit 4 style? (ie using annotations instead of extending TestCase)
          • Can you add a test case for a volume that is not in vols?
          • Rather than sleeping in the test case, can you call shutdown and then awaitTermination() with a longer timeout? This should ensure that the test is allowed to run longer if the machine is super slow, but doesn't waste time if not necessary. This would also make you add an awaitTermination() call, which I think is a good thing to expose.
          • What's with the changes to FsShell? Was this accidental?
          • Missing license header on AsyncDIskService.java
          • The comment still says "In the future, we should put it in common". Since this is a common jira, please remove that
          • IMO the thread counts per volume should be configurable. Some systems can handle plenty of concurrent metadata ops while others, not so much. I don't feel strongly about this, though - happy to leave it to a later JIRA if people want this.
          • Should the threads be made daemon threads? I'm not sure what the expected behavior should be when the server gets SIGINTed, but I think if they're not daemon threads it will block server shutdown.

          Some of the above comments probably apply to the existing code, since this is just a move. If so, it seems fine to file a new JIRA for these improvements so we can move forward with the main thrust of this issue.

          Thanks!

          Show
          Todd Lipcon added a comment - Hi Zheng, Can you convert the unit test to JUnit 4 style? (ie using annotations instead of extending TestCase) Can you add a test case for a volume that is not in vols? Rather than sleeping in the test case, can you call shutdown and then awaitTermination() with a longer timeout? This should ensure that the test is allowed to run longer if the machine is super slow, but doesn't waste time if not necessary. This would also make you add an awaitTermination() call, which I think is a good thing to expose. What's with the changes to FsShell? Was this accidental? Missing license header on AsyncDIskService.java The comment still says "In the future, we should put it in common". Since this is a common jira, please remove that IMO the thread counts per volume should be configurable. Some systems can handle plenty of concurrent metadata ops while others, not so much. I don't feel strongly about this, though - happy to leave it to a later JIRA if people want this. Should the threads be made daemon threads? I'm not sure what the expected behavior should be when the server gets SIGINTed, but I think if they're not daemon threads it will block server shutdown. Some of the above comments probably apply to the existing code, since this is just a move. If so, it seems fine to file a new JIRA for these improvements so we can move forward with the main thrust of this issue. Thanks!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12427667/HADOOP-6433.1.patch
          against trunk revision 889378.

          +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 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 1 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-h4.grid.sp2.yahoo.net/194/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/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/12427667/HADOOP-6433.1.patch against trunk revision 889378. +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 release audit. The applied patch generated 2 release audit warnings (more than the trunk's current 1 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-h4.grid.sp2.yahoo.net/194/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/194/console This message is automatically generated.
          Hide
          Zheng Shao added a comment -

          This patch contains the new class and a test.

          Show
          Zheng Shao added a comment - This patch contains the new class and a test.

            People

            • Assignee:
              Zheng Shao
              Reporter:
              Zheng Shao
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development