Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1865

Share LeaseChecker thread among DFSClients

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Each DFSClient runs a LeaseChecker thread within a JVM. The number threads could be reduced by sharing the threads.

      1. h1865_20110509.patch
        26 kB
        Tsz Wo Nicholas Sze
      2. h1865_20110508b.patch
        32 kB
        Tsz Wo Nicholas Sze
      3. h1865_20110508.patch
        31 kB
        Tsz Wo Nicholas Sze
      4. h1865_20110504.patch
        17 kB
        Tsz Wo Nicholas Sze
      5. h1865_20110503.patch
        17 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          dhruba borthakur added a comment -

          why not share the DFSClient itself?

          Show
          dhruba borthakur added a comment - why not share the DFSClient itself?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have considered share DFSClient directly. However, DFSClient depends on conf parameters like timeout, packet size, etc. So it cannot be shared.

          Show
          Tsz Wo Nicholas Sze added a comment - I have considered share DFSClient directly. However, DFSClient depends on conf parameters like timeout, packet size, etc. So it cannot be shared.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Dhruba, question to you: DFSClient has a unique client name even there are multiple DFSClient instances in a JVM. Do you think that we can use the same name for all DFSClient instances?

          Show
          Tsz Wo Nicholas Sze added a comment - Dhruba, question to you: DFSClient has a unique client name even there are multiple DFSClient instances in a JVM. Do you think that we can use the same name for all DFSClient instances?
          Hide
          dhruba borthakur added a comment -

          hi nicholas, I do not undertand your question/proposal. A single JVM can have multiple DFSClients, each talking to different namenodes. are you proposing that there be one thread to renew lease to all the namenodes?

          Show
          dhruba borthakur added a comment - hi nicholas, I do not undertand your question/proposal. A single JVM can have multiple DFSClients, each talking to different namenodes. are you proposing that there be one thread to renew lease to all the namenodes?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I am actually thinking to have one thread per namenode per user. In such case, could we use the same client name for all DFSClient instances?

          Show
          Tsz Wo Nicholas Sze added a comment - I am actually thinking to have one thread per namenode per user. In such case, could we use the same client name for all DFSClient instances?
          Hide
          dhruba borthakur added a comment -

          Oh ok, can you also explain the usecase why you have multiple instances of DFSClient inside the same jvm (aren't all config properties for file creation already part of the FileSystem.create api?)

          Show
          dhruba borthakur added a comment - Oh ok, can you also explain the usecase why you have multiple instances of DFSClient inside the same jvm (aren't all config properties for file creation already part of the FileSystem.create api?)
          Hide
          Devaraj Das added a comment -

          HDFS-1131 is one use case (maybe that jira can be closed as a duplicate of this one).

          Show
          Devaraj Das added a comment - HDFS-1131 is one use case (maybe that jira can be closed as a duplicate of this one).
          Hide
          dhruba borthakur added a comment -

          Ok, got it.

          Show
          dhruba borthakur added a comment - Ok, got it.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h1865_20110503.patch: 1st patch, need some new tests.

          Show
          Tsz Wo Nicholas Sze added a comment - h1865_20110503.patch: 1st patch, need some new tests.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h1865_20110504.patch: updated with trunk.

          Show
          Tsz Wo Nicholas Sze added a comment - h1865_20110504.patch: updated with trunk.
          Hide
          Suresh Srinivas added a comment -

          Comments:

          1. DFSClient.java
            • DFSClient#filesBeingWritten - worth adding a comment that a file can be opened for write by a DFSClient only once.
          2. LeaseRenewer.java
            • In the class document, it is good to describe lease mechanism briefly. This would help people understand the code better. Adding additional comments on how LeaseRenewer works would also be good. Adding information about synchronization will also help understand the code better.
            • Please throw HadoopIllegalArgumentException instead of NullPointerException.
            • Please do not access DFSClient#filesBeingWritten in this class. Add methods to DFSClient to do this.
            • Factory#remove - please avoid adding synchronized on renewer. Instead call a method on renewer, that is synchronized.
            • Minor: Simplify the code in in LeaseRenewer#get(). If map returns empty then add it to map and return it. You do not not need if else. You can do it with just if and can get rid of "r".
            • Copy on array list for DFSClients as an alternative to making a copy every time in renew()?
            • Why do you check {if (r == stored) }

              in Factory#remove()?

            • Why did we choose to use volatile for sleepPeriod and synchronized for gracePeriod and other periods?
          Show
          Suresh Srinivas added a comment - Comments: DFSClient.java DFSClient#filesBeingWritten - worth adding a comment that a file can be opened for write by a DFSClient only once. LeaseRenewer.java In the class document, it is good to describe lease mechanism briefly. This would help people understand the code better. Adding additional comments on how LeaseRenewer works would also be good. Adding information about synchronization will also help understand the code better. Please throw HadoopIllegalArgumentException instead of NullPointerException. Please do not access DFSClient#filesBeingWritten in this class. Add methods to DFSClient to do this. Factory#remove - please avoid adding synchronized on renewer. Instead call a method on renewer, that is synchronized. Minor: Simplify the code in in LeaseRenewer#get(). If map returns empty then add it to map and return it. You do not not need if else. You can do it with just if and can get rid of "r". Copy on array list for DFSClients as an alternative to making a copy every time in renew()? Why do you check {if (r == stored) } in Factory#remove()? Why did we choose to use volatile for sleepPeriod and synchronized for gracePeriod and other periods?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h1865_20110508.patch: added a test and incorporated Suresh's comments.

          > Why do you check

          {if (r == stored) }

          in Factory#remove()?

          Since a renewer may expire, the stored renewer can be different. Added a comment in the code.

          > Why did we choose to use volatile for sleepPeriod and synchronized for gracePeriod and other periods?

          Used synchronized for both now.

          Show
          Tsz Wo Nicholas Sze added a comment - h1865_20110508.patch: added a test and incorporated Suresh's comments. > Why do you check {if (r == stored) } in Factory#remove()? Since a renewer may expire, the stored renewer can be different. Added a comment in the code. > Why did we choose to use volatile for sleepPeriod and synchronized for gracePeriod and other periods? Used synchronized for both now.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12478552/h1865_20110508.patch
          against trunk revision 1100811.

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

          +1 tests included. The patch appears to include 4 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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.cli.TestHDFSCLI
          org.apache.hadoop.hdfs.TestDFSShell
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader
          org.apache.hadoop.hdfs.TestReadWhileWriting

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//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/12478552/h1865_20110508.patch against trunk revision 1100811. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.cli.TestHDFSCLI org.apache.hadoop.hdfs.TestDFSShell org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader org.apache.hadoop.hdfs.TestReadWhileWriting +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/467//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h1865_20110508b.patch: fixed TestReadWhileWriting. The other failed tests are not related.

          Show
          Tsz Wo Nicholas Sze added a comment - h1865_20110508b.patch: fixed TestReadWhileWriting . The other failed tests are not related.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12478562/h1865_20110508b.patch
          against trunk revision 1100811.

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

          +1 tests included. The patch appears to include 7 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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.cli.TestHDFSCLI
          org.apache.hadoop.hdfs.TestDFSShell
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//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/12478562/h1865_20110508b.patch against trunk revision 1100811. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.cli.TestHDFSCLI org.apache.hadoop.hdfs.TestDFSShell org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/468//console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Minor comments:

          1. I prefer to retain using "DFSClient" instead of simple class name while building the client name.
          2. TestLease could be simplified using mockito
          3. Javadoc could be more descriptive for LeaseRenewer
          Show
          Suresh Srinivas added a comment - Minor comments: I prefer to retain using "DFSClient" instead of simple class name while building the client name. TestLease could be simplified using mockito Javadoc could be more descriptive for LeaseRenewer
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h1865_20110509.patch: incorporated the minor comments.

          Show
          Tsz Wo Nicholas Sze added a comment - h1865_20110509.patch: incorporated the minor comments.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12478639/h1865_20110509.patch
          against trunk revision 1101137.

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

          +1 tests included. The patch appears to include 7 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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.server.namenode.TestUnderReplicatedBlocks
          org.apache.hadoop.hdfs.TestDFSShell
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileConcurrentReader

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//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/12478639/h1865_20110509.patch against trunk revision 1101137. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.server.namenode.TestUnderReplicatedBlocks org.apache.hadoop.hdfs.TestDFSShell org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestFileConcurrentReader +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/469//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The failure TestUnderReplicatedBlocks was caused by timeout; see HDFS-342. I can reproduce the failure in my machine without the patch.

          The failure TestFiDataTransferProtocol2 was caused by NullPointerException in ipc.Server; see HADOOP-7270.

          The other failed tests are known.

          Show
          Tsz Wo Nicholas Sze added a comment - The failure TestUnderReplicatedBlocks was caused by timeout; see HDFS-342 . I can reproduce the failure in my machine without the patch. The failure TestFiDataTransferProtocol2 was caused by NullPointerException in ipc.Server; see HADOOP-7270 . The other failed tests are known.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The failure TestUnderReplicatedBlocks was caused by timeout; see HDFS-342. I can reproduce the failure in my machine without the patch.

          The failure TestFiDataTransferProtocol2 was caused by NullPointerException in ipc.Server; see HADOOP-7270.

          The other failed tests are known.

          Show
          Tsz Wo Nicholas Sze added a comment - The failure TestUnderReplicatedBlocks was caused by timeout; see HDFS-342 . I can reproduce the failure in my machine without the patch. The failure TestFiDataTransferProtocol2 was caused by NullPointerException in ipc.Server; see HADOOP-7270 . The other failed tests are known.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Thanks Suresh for the reviews.

          I have committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - Thanks Suresh for the reviews. I have committed this.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #637 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/637/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #637 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/637/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/ )

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development