Details

    • Hadoop Flags:
      Reviewed

      Description

      Based on the discussion of HDFS-5923, the ACL bit is no longer passed to the client directly. Ls should call getAclStatus() instead since it needs to display the ACL bit as a part of the permission.

      1. HDFS-5932.1.patch
        4 kB
        Chris Nauroth
      2. HDFS-5932.2.patch
        4 kB
        Chris Nauroth
      3. HDFS-5932.3.patch
        9 kB
        Chris Nauroth
      4. HDFS-5932.4.patch
        9 kB
        Chris Nauroth

        Issue Links

          Activity

          Hide
          Haohui Mai added a comment -

          I've committed the patch. Thanks Chris for the contribution.

          Show
          Haohui Mai added a comment - I've committed the patch. Thanks Chris for the contribution.
          Hide
          Haohui Mai added a comment -

          Thanks very much for the explanation. +1. I'll commit shortly.

          Show
          Haohui Mai added a comment - Thanks very much for the explanation. +1. I'll commit shortly.
          Hide
          Chris Nauroth added a comment -

          I'm attaching patch version 4.

          It's a little bit weird to unwrap RpcNoSuchMethodException in DFSClient.

          Thanks for bringing this up. I meant to discuss it earlier. I agree that it's inconsistent to unwrap this in the client. Patch v4 goes back to unwrapping inside ls.

          The unit test can be greatly simplified using mockito, but it can be addressed in a separate jira.

          The trouble here is that Mockito decorates instances, not classes. For the shell, the FileSystem cache takes responsbility for creating the instance internally via reflection. We don't have direct control of the instance it gets, only the class definition specified via configuration. To test that the shell executes successfully (exit code 0), it seems this is what we have to do. I don't see a way to use Mockito, but if you have an idea, let me know.

          Show
          Chris Nauroth added a comment - I'm attaching patch version 4. It's a little bit weird to unwrap RpcNoSuchMethodException in DFSClient. Thanks for bringing this up. I meant to discuss it earlier. I agree that it's inconsistent to unwrap this in the client. Patch v4 goes back to unwrapping inside ls. The unit test can be greatly simplified using mockito, but it can be addressed in a separate jira. The trouble here is that Mockito decorates instances, not classes. For the shell, the FileSystem cache takes responsbility for creating the instance internally via reflection. We don't have direct control of the instance it gets, only the class definition specified via configuration. To test that the shell executes successfully (exit code 0), it seems this is what we have to do. I don't see a way to use Mockito, but if you have an idea, let me know.
          Hide
          Haohui Mai added a comment -
           +                                     RpcNoSuchMethodException.class,
                                                UnresolvedPathException.class);
          

          It's a little bit weird to unwrap RpcNoSuchMethodException in DFSClient. Comparing the v2 and v3 patch, it might be better to stick with the v2 version.

          The unit test can be greatly simplified using mockito, but it can be addressed in a separate jira.

          Other than that the patch looks pretty good.

          Show
          Haohui Mai added a comment - + RpcNoSuchMethodException.class, UnresolvedPathException.class); It's a little bit weird to unwrap RpcNoSuchMethodException in DFSClient . Comparing the v2 and v3 patch, it might be better to stick with the v2 version. The unit test can be greatly simplified using mockito, but it can be addressed in a separate jira. Other than that the patch looks pretty good.
          Hide
          Chris Nauroth added a comment -

          Thanks for the review, Haohui. I'm attaching patch version 3.

          Can you just catch the RpcNoSuchMethodException?

          To be able to catch it directly, DFSClient must do the unwrapping, so I've included that change.

          I also wonder whether it is possible to cache the fs object directly instead.

          I considered this, but it would put the logic at risk of failing for FileSystem subclasses that have a misbehaving hashCode or equals. We don't define these in the base class, and I'm not aware of any requirement we've placed on custom implementations that they must override them. URI on the other hand is already used in the FileSystem cache key, so we already have an implicit assumption in the code (for better or worse) that subclasses must return a reasonable URI.

          Show
          Chris Nauroth added a comment - Thanks for the review, Haohui. I'm attaching patch version 3. Can you just catch the RpcNoSuchMethodException ? To be able to catch it directly, DFSClient must do the unwrapping, so I've included that change. I also wonder whether it is possible to cache the fs object directly instead. I considered this, but it would put the logic at risk of failing for FileSystem subclasses that have a misbehaving hashCode or equals . We don't define these in the base class, and I'm not aware of any requirement we've placed on custom implementations that they must override them. URI on the other hand is already used in the FileSystem cache key, so we already have an implicit assumption in the code (for better or worse) that subclasses must return a reasonable URI.
          Hide
          Haohui Mai added a comment -
          +    if (aclNotSupportedFsSet.contains(fs.getUri())) {
          +      // This FileSystem failed to run the ACL API in an earlier iteration.
          +      return false;
          +    }
          +    try {
          +      return !fs.getAclStatus(item.path).getEntries().isEmpty();
          +    } catch (RemoteException e) {
          +      // If this is a RpcNoSuchMethodException, then the client is connected to
          +      // an older NameNode that doesn't support ACLs.  Keep going.
          +      IOException e2 = e.unwrapRemoteException(RpcNoSuchMethodException.class);
          +      if (!(e2 instanceof RpcNoSuchMethodException)) {
          +        throw e;
          +      }
          +    } catch (IOException e) {
          +      // The NameNode supports ACLs, but they are not enabled.  Keep going.
          +      String message = e.getMessage();
          +      if (message != null && !message.contains("ACLs has been disabled")) {
          +        throw e;
          +      }
          +    } catch (UnsupportedOperationException e) {
          +      // The underlying FileSystem doesn't implement ACLs.  Keep going.
          +    }
          +    // Remember that this FileSystem cannot support ACLs.
          +    aclNotSupportedFsSet.add(fs.getUri());
          +    return false;
          

          This method is a little bit confusing. Can you just catch the RpcNoSuchMethodException:

          try {
            getFileStatus();
          } catch (RpcNoSuchMethodException) {
            unsupportedFs.add(...);
          }
          return false;
          

          I also wonder whether it is possible to cache the fs object directly instead.

          Do you want to add a unit test to make sure that ls works as expected when RpcNoSuchMethodException is thrown?

          Show
          Haohui Mai added a comment - + if (aclNotSupportedFsSet.contains(fs.getUri())) { + // This FileSystem failed to run the ACL API in an earlier iteration. + return false ; + } + try { + return !fs.getAclStatus(item.path).getEntries().isEmpty(); + } catch (RemoteException e) { + // If this is a RpcNoSuchMethodException, then the client is connected to + // an older NameNode that doesn't support ACLs. Keep going. + IOException e2 = e.unwrapRemoteException(RpcNoSuchMethodException.class); + if (!(e2 instanceof RpcNoSuchMethodException)) { + throw e; + } + } catch (IOException e) { + // The NameNode supports ACLs, but they are not enabled. Keep going. + String message = e.getMessage(); + if (message != null && !message.contains( "ACLs has been disabled" )) { + throw e; + } + } catch (UnsupportedOperationException e) { + // The underlying FileSystem doesn't implement ACLs. Keep going. + } + // Remember that this FileSystem cannot support ACLs. + aclNotSupportedFsSet.add(fs.getUri()); + return false ; This method is a little bit confusing. Can you just catch the RpcNoSuchMethodException : try { getFileStatus(); } catch (RpcNoSuchMethodException) { unsupportedFs.add(...); } return false ; I also wonder whether it is possible to cache the fs object directly instead. Do you want to add a unit test to make sure that ls works as expected when RpcNoSuchMethodException is thrown?
          Hide
          Chris Nauroth added a comment -

          I'm attaching patch version 2 to address the additional case I described in the last comment.

          Show
          Chris Nauroth added a comment - I'm attaching patch version 2 to address the additional case I described in the last comment.
          Hide
          Chris Nauroth added a comment -

          There is one more case I need to consider: the getAclStatus RPC endpoint exists, but ACLs are disabled in configuration. I'll test that and update the patch accordingly tomorrow.

          Show
          Chris Nauroth added a comment - There is one more case I need to consider: the getAclStatus RPC endpoint exists, but ACLs are disabled in configuration. I'll test that and update the patch accordingly tomorrow.
          Hide
          Chris Nauroth added a comment -

          Also, I tested this change against a "legacy" NameNode built from trunk to make sure the compatibility logic was working as intended.

          Show
          Chris Nauroth added a comment - Also, I tested this change against a "legacy" NameNode built from trunk to make sure the compatibility logic was working as intended.
          Hide
          Chris Nauroth added a comment -

          I'm attaching the patch. This is dependent on HDFS-5923, so you'd need to have that patch applied first.

          This detects the presence of an ACL by calling getAclStatus. For compatibility with an older NameNode, we trap the RPC exception if getAclStatus is not defined on the server side. We keep track of the URI of any FileSystem instance for which the getAclStatus method was not defined. This prevents spamming multiple RPC calls that we know can't possibly succeed.

          Show
          Chris Nauroth added a comment - I'm attaching the patch. This is dependent on HDFS-5923 , so you'd need to have that patch applied first. This detects the presence of an ACL by calling getAclStatus . For compatibility with an older NameNode, we trap the RPC exception if getAclStatus is not defined on the server side. We keep track of the URI of any FileSystem instance for which the getAclStatus method was not defined. This prevents spamming multiple RPC calls that we know can't possibly succeed.

            People

            • Assignee:
              Chris Nauroth
              Reporter:
              Haohui Mai
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development