Hadoop Common
  1. Hadoop Common
  2. HADOOP-6640

FileSystem.get() does RPC retries within a static synchronized block

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: fs
    • Labels:
      None
    • Environment:

      all

    • Hadoop Flags:
      Reviewed

      Description

      If using FileSystem.get() in a multithreaded environment, and one get() locks because the NN URI is too slow or not responding and retries are in progress, all other get() (for the diffferent users, NN) are blocked.

      the synchronized block in in the static instance of Cache inner class.

      1. getFS.patch
        4 kB
        Hairong Kuang
      2. getFS_yahoo20s.patch
        4 kB
        Hairong Kuang

        Issue Links

          Activity

          Hide
          Hairong Kuang added a comment -

          This patch is for Yahoo's 0.20s branch.

          Show
          Hairong Kuang added a comment - This patch is for Yahoo's 0.20s branch.
          Hide
          Hairong Kuang added a comment -

          HADOOP-6691 fixed a bug in the test unit test in the committed patch.

          Show
          Hairong Kuang added a comment - HADOOP-6691 fixed a bug in the test unit test in the committed patch.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #294 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/294/)
          . FileSystem.get() does RPC retries within a static synchronized block. Contributed by Hairong Kuang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #294 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/294/ ) . FileSystem.get() does RPC retries within a static synchronized block. Contributed by Hairong Kuang.
          Hide
          Hairong Kuang added a comment -

          I've just committed this!

          Show
          Hairong Kuang added a comment - I've just committed this!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #213 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/213/)
          . FileSystem.get() does RPC retries within a static synchronized block. Contributed by Hairong Kuang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #213 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/213/ ) . FileSystem.get() does RPC retries within a static synchronized block. Contributed by Hairong Kuang.
          Hide
          Hadoop QA added a comment -

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

          +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/438/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/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/12440376/getFS.patch against trunk revision 927979. +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/438/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/438/console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... If you insist on using solution 2, I am not against it.

          For this issue, I am not insist on solution 2 but I do think that solution 2 has its value. We should implement it when we have bandwidth.

          Show
          Tsz Wo Nicholas Sze added a comment - > ... If you insist on using solution 2, I am not against it. For this issue, I am not insist on solution 2 but I do think that solution 2 has its value. We should implement it when we have bandwidth.
          Hide
          Hairong Kuang added a comment -

          From my point of view, what's fundamentally wrong is that version check is performed when a rpc proxy is created. Instead, it should be performed either when a connection to a rpc server is created or whenever a rpc is received at the server side. So neither solution 1 and solution 2 solves the fundamental problem and it does not matter which solution we use. If you insist on using solution 2, I am not against it.

          Show
          Hairong Kuang added a comment - From my point of view, what's fundamentally wrong is that version check is performed when a rpc proxy is created. Instead, it should be performed either when a connection to a rpc server is created or whenever a rpc is received at the server side. So neither solution 1 and solution 2 solves the fundamental problem and it does not matter which solution we use. If you insist on using solution 2, I am not against it.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          FYI, solution 2 also fix problems like HADOOP-2835: "hadoop fs -help ..." should not require a NameNode to show help messages.

          Show
          Tsz Wo Nicholas Sze added a comment - FYI, solution 2 also fix problems like HADOOP-2835 : "hadoop fs -help ..." should not require a NameNode to show help messages.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Hide
          Hairong Kuang added a comment -

          Here is a patch that uses solution 1, together with a unit test. The unit test hangs in the trunk but passes with the fix.

          Show
          Hairong Kuang added a comment - Here is a patch that uses solution 1, together with a unit test. The unit test hangs in the trunk but passes with the fix.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have no problem if you want to implement solution 1.

          Show
          Tsz Wo Nicholas Sze added a comment - I have no problem if you want to implement solution 1.
          Hide
          Hairong Kuang added a comment -

          This solution will add cost to each rpc call although the cost is insignificant. Base on the discussion so far, will you consider solution 1 (Sanjay's proposal)? From my point of view, solution 1 is quite straightforward and the concern you have seems very unlikely to happen.

          Show
          Hairong Kuang added a comment - This solution will add cost to each rpc call although the cost is insignificant. Base on the discussion so far, will you consider solution 1 (Sanjay's proposal)? From my point of view, solution 1 is quite straightforward and the concern you have seems very unlikely to happen.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Yes, it needs to check a boolean like the one in DFSClient.checkOpen().

          Show
          Tsz Wo Nicholas Sze added a comment - Yes, it needs to check a boolean like the one in DFSClient.checkOpen().
          Hide
          Hairong Kuang added a comment -

          Then this means every future RPC call of DFSClient requires a check if a DFSClient has been initialized or not.

          Show
          Hairong Kuang added a comment - Then this means every future RPC call of DFSClient requires a check if a DFSClient has been initialized or not.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > The RPC is issued by the RPC client to verify the version # when creating a RPC proxy. I am not sure if it is a good idea to delay this until there is an application issued RPC. This would require a change to RPC implementation.

          How about delay the RPC proxy initialization? i.e. when creating a DistributedFileSystem, it does not create the RPC proxy to Namenode (or even not creating a DFSClient.)

          Show
          Tsz Wo Nicholas Sze added a comment - > The RPC is issued by the RPC client to verify the version # when creating a RPC proxy. I am not sure if it is a good idea to delay this until there is an application issued RPC. This would require a change to RPC implementation. How about delay the RPC proxy initialization? i.e. when creating a DistributedFileSystem, it does not create the RPC proxy to Namenode (or even not creating a DFSClient.)
          Hide
          Hairong Kuang added a comment -

          > Change DistributedFileSystem so that it does a lazy connection: it defers connecting to the server until there is an rpc.
          The RPC is issued by the RPC client to verify the version # when creating a RPC proxy. I am not sure if it is a good idea to delay this until there is an application issued RPC. This would require a change to RPC implementation.

          Show
          Hairong Kuang added a comment - > Change DistributedFileSystem so that it does a lazy connection: it defers connecting to the server until there is an rpc. The RPC is issued by the RPC client to verify the version # when creating a RPC proxy. I am not sure if it is a good idea to delay this until there is an application issued RPC. This would require a change to RPC implementation.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          When FileSystem cache is enabled, FileSystem.get(..) will call FileSystem.Cache.get(..), which is a synchronized method. If the lookup fails, a new instance will be initialized. Depends on the FileSystem subclass implementation, the initialization may take a long time. In such case, the FileSystem.Cache lock will be hold and all calls to FileSystem.get(..) by other threads will be blocked for a long time.

          In particular, the DistributedFileSystem initialization may take a long time since there are retries. It is even worst if the socket timeout is set to a large value.

          There are two possible fixes for the problem:

          1. (by Sanjay) Change FileSystem.Cache.get(..) so that if the lookup fails, it first releases the lock, initializes a FileSystem instance, acquires the lock again, and then add the instance to the cache. One problem is that if a user application keeps calling FileSystem.get(..) for the same FileSystem in a short period of time, it will result in initializing many instances.
          1. Change DistributedFileSystem so that it does a lazy connection: it defers connecting to the server until there is an rpc. A drawback is that this only fixes DistributedFileSystem but not other FileSystem subclasses.
          Show
          Tsz Wo Nicholas Sze added a comment - When FileSystem cache is enabled, FileSystem.get(..) will call FileSystem.Cache.get(..), which is a synchronized method. If the lookup fails, a new instance will be initialized. Depends on the FileSystem subclass implementation, the initialization may take a long time. In such case, the FileSystem.Cache lock will be hold and all calls to FileSystem.get(..) by other threads will be blocked for a long time. In particular, the DistributedFileSystem initialization may take a long time since there are retries. It is even worst if the socket timeout is set to a large value. There are two possible fixes for the problem: (by Sanjay) Change FileSystem.Cache.get(..) so that if the lookup fails, it first releases the lock, initializes a FileSystem instance, acquires the lock again, and then add the instance to the cache. One problem is that if a user application keeps calling FileSystem.get(..) for the same FileSystem in a short period of time, it will result in initializing many instances. Change DistributedFileSystem so that it does a lazy connection: it defers connecting to the server until there is an rpc. A drawback is that this only fixes DistributedFileSystem but not other FileSystem subclasses.

            People

            • Assignee:
              Hairong Kuang
              Reporter:
              Alejandro Abdelnur
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development