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.

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

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.

I'm attaching patch version 2 to address the additional case I described in the last 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?

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.

 +                                     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.

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.

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

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