Add extended attributes (XAttr) functionality

[weilintsaiWand]

Implement extended attributes for Giraffa.

[weilintsaiWand]

Summary of changes for init commit:

• Add new CF "xAttrColumnFamily"
• XAttr related methods done with 40 unit tests
• The prefix (such as "user.") will inserted to HBase, too. For instance, there's a column named "user.attr1"
• Add some methods in INodeManager. Do not change/use any existed methods in INodeManager. That is to say, getINode and updateINode does not load/set any XAttr related data.

To do

• Add permission checking.
• Add configuration checking and its tests (maybe in TestGiraffaFSNegative)
• Add java doc

Not sure parts

• Merge XAttr to INode. For instance, merge the new features to getINode updateINode.
• Does NameSpace matter?
• Do we need logging when error occur?

[weilintsaiWand]

Changes for second commit

Add logic to check if DFS_NAMENODE_XATTRS_ENABLED_KEY is disabled The logic is in NamespaceProcessor.java. Hence, we will have some permission checking logic in NamespaceProcessor.java and some are in XAttrOp.java. It's possible to let them all in XAttrOp.java but I choose current approach for now.

[weilintsaiWand]

Update Summary

• Add new CF "xattrs"
• Units are ready
• The prefix (such as "user.") will inserted to HBase, too. For instance, there's a column named "user.attr1"
• Add some methods in INodeManager. Do not change/use any existed methods in INodeManager. That is to say, getINode and updateINode does not load/set any XAttr related data.
• Add permission checking.
• Add configuration checking and its tests ( TestGiraffaFSNegative)

What we checked

• Check if XAttribute feature is enable (Checked in NamespaceProcessor)
• Check for XAttribute permission (system/security/trusted/user)
• Check for path permission (do we have write permission on specific path)
• Check if flag is valid
• Check attribute size (max len of name and value) (Checked in NamespaceProcessor)
• Check if exceed limit size of attr for given node

What we didn't support

• Check if safe mode (Not supported now)
• ACL related method (Not supported now)
• Check for file system permission (does anyone blocking the file system?) (read lock/write lock)

What is different

• While set/remove attr:
  Hadoop: Check node's W permission and all parents X permission
  Giraffa: Check node's W permission and its parent X permission.

What need review

Tests

• Need review all tests to decide which parts to be removed

[pjeli]

• Everywhere: replace "xattr" with "xAttr".
• In BlockManagementAgent: change comment to say "If not File Attributes related then skip processing".
• INodeManager, remove comment about "put a cell directly". That is obvious enough by the logic.
• TestGiraffaFSNegative: extra new line before startMiniCluster().
• TestGiraffaFSNegative: don't use assertTrue(false). Use either fail() or rewrite test with "(expected=Class.class)".
• XAttrPermissionFilter: extra space in checkPermissionForApi().
• XAttrPermissionFilter: why while loops? Use a for each loop.
• XAttrPermissionFilter: Use Precondictions.checkNotNull().
• XAttrPermissionFilter: Remove newline after if(!xAttrs.isEmpty()).
• XAttrPermissionFilter: filterXAttrsForApi, replace assert with Preconditions.checkNotNull().
• XAttrOp: please make more use of Preconditions and Predicates in order to minimize null checking and filtering done here.
• XAttrOp should have better Javadoc. It is not very detailed at the moment. Explain the differences.

Let's fix these first and then I'll do another review.

[pjeli]

• copyWithRenameFlag() does setXAttrs AFTER updateINode. This is an issue because if the RegionServer fails after the updateINode but before the setXAttrs, then the XAttrs will never be copied into the new row. We need to make updateINode(dstNode) update the XAttrs at the same time. Look into adding an updateINode(dstNode, List) method perhaps.
• XAttrPermissionFilter: filterXAttrsForApi: filteredXAttrs -> should have generics in order to avoid raw type issues.
• TestXAttr: Can we uncomment the tests?
• TestXAttr: In cases where you don't do anything with the catched Exception you should name the Exception variable from e -> ignored.

Let's fix those and then I'll do another review.

[weilintsaiWand]

Fix done and ready for review.

[shvachko]

Hey Wei-Lin. I noticed there is bunch of warnings in TestXAttr. They are related to that you do not parametrize PrivilegedExceptionAction. The parameter should be the same as the return value of run() method. It seems that you do not need to return anything. In this case the common practice is to use Void. E.g.:

new PrivilegedExceptionAction<Void>() {
  public Void run() throws Exception {
    return null;
  }
}

Plamen, let me know if this is ready otherwise.

[weilintsaiWand]

Thanks Konstantin, To be honest, the code style is copied from Hadoop. Since I never used PrivilegedExceptionAction before, I was not aware that it's a generic . I also found Intellij didn't report warnings for non-parametrized situation so I will try to enable this checking in my Intellij. Hopefully it would not happen again.

The fix is ready for your reference. Please help check it. Thanks.

[shvachko]

This looks good. Couple more nits, while we are waiting for Plamen's resolution.

1. Let's replace Lists.newArrayListWithCapacity with new ArrayList<XAttr>(int). You used google.common in several places. We should stick to java types and libraries, rather than introducing new dependencies as I said in another issue.
2. We should move XAttrOp and XAttrPermissionFilter under org.apache.giraffa package, because there is nothing hbase specific there. A general rule of thumb is to keep as much as possible outside of hbase and hdfs dependent packages.
   • Except for HBaseRpcUtil, which is used to obtain FSPermissionChecker. I suggest you use NamespaceProcessor.getFsPermissionChecker() to obtain the checker. You may pass it as parameter to XAttrOp methods. Then you can remove a few members XAttrOp.fsOwnerShortUserName and supergroup.

[weilintsaiWand]

Thanks Konstantin for the dependencies reminder, I checked the code and found in NamespaceProcessor I use int size = xAttr.getName().getBytes(Charsets.UTF_8).length; which I copied from Hadoop. I am planning to replace this line with int size = xAttr.getName().getBytes().length; so we don't need import com.google.common.base.Charsets; Please correct me if I was wrong

[weilintsaiWand]

Fix for removing Guava dependencies and reorder XAttrOp and XAttrPermissionFilter done.

[shvachko]

Yes, this is what I meant. +1

[shvachko]

Just finished running the test suite and found one more thing. At the end of the log for TestXAtt I see several ConnectException: Connection refused. This means the test is not closing something that it opened. This exception persists through subsequent tests, which means this thread from TestXAtt keeps trying to connect to ZK within the following tests. Should investigate this.

15/07/20 10:54:35 INFO zookeeper.ClientCnxn: Opening socket connection to server localhost/127.0.0.1:54434. Will not attempt to authenticate using SASL (unknown error)
15/07/20 10:54:35 WARN zookeeper.ClientCnxn: Session 0x14eac98edb700c3 for server null, unexpected error, closing socket connection and attempting reconnect
java.net.ConnectException: Connection refused
    at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
    at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:692)
    at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
    at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081)

[weilintsaiWand]

Seems like there are some flaws in my Mock parts. Hence, something is open when we change the user to verify permission parts any may not close properly after test finished. Still working on it now.

[weilintsaiWand]

Fix and refactor the code to use a private static variable to keep a FS opened by child-thread. Hence, the FS open/close operation can be handled properly.

[pjeli]

This looks good. I don't see anymore "Connection refused" exceptions. +1 from my end.

[shvachko]

I just pushed some more changes to your branch. The main thing that I removed redundant member TestXAttr.grfs, renamed anotherThreadFs to user1fs and made it initialized explicitly in beforeClass(), also reused member conf. Hope this simplified things. One last thing to fix is that when I run the test in Eclipse it incorrectly creates build directory under giraffa-core. build directory should be under giraffa-core/target, otherwise it is not cleaned up by mvn clean target.

[shvachko]

We can just clean up the directory as in https://issues.apache.org/jira/browse/HDFS-7561

[weilintsaiWand]

The build directory is created in base test's @BeforeClass void init() method. It's hard to change the behavior because all Mock/Override/Configuration Setting take place after base class's @BeforeClass method. As a result, we use a work around solution to deleted build directory manually in our @AfterClass method. I also removed Mock of init since it's useless.

[weilintsaiWand]

After Konstantin's simplification, we have shared user1fs among all tests. Hence, it seems like we don't need user1.doAs in each test. As a result, I refine the code to remove that part in the latest commit. Please correct me if I was wrong.

[shvachko]

Yes, good point, Wei-Lin. Sounds like we perfected this patch quite well. Committing now.

[shvachko]

I just committed this. Thank you Wei-Lin.