[
https://issues.apache.org/jira/browse/HDFS-6258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13985877#comment-13985877
]
Uma Maheswara Rao G commented on HDFS-6258:
-------------------------------------------
Thanks a lot Charles for the review!
I also did my initial review on this patch, please find my comments below along
with Charles comments.
DFSConfigKeys.java:
{code}
public static final String DFS_NAMENODE_XATTRS_MAX_LIMIT_KEY =
"dfs.namenode.xattrs.max-limit";
+ public static final int DFS_NAMENODE_XATTRS_MAX_LIMIT_DEFAULT = 32;
{code}
Do we need to mention per Inode max limit?
Tests:
I think we need test cases for verifying inmemory xattrs updations and getting
them back. Which is kind of integrations test from client side to XattrFeature
usage.
ConfigFlag.java:
How about having NNConf like DNConf which will load the config on statup and
have the check methods there?
{code}
/**
* Simple class encapsulating all of the configuration that the DataNode
* loads at startup time.
*/
@InterfaceAudience.Private
public class DNConf {
{code}
{code}
+ XAttrStorage.updateINodeXAttrs(inode, newXAttrs, snapshotId);
+
+ return newXAttrs;
EnumSet<XAttrSetFlag> flag) throws IOException {
+
+ assert hasWriteLock();
{code}
You don't need empty line here.
FSDirectory.java
{code}
if (xAttrs.size() > xAttrsLimit) {
+ throw new IOException("Operation fails, XAttrs of " +
+ "inode exceeds maximum limit of " + xAttrsLimit);
+ }
{code}
Please remove tab characters above.
FSNamesystem.java:
{code}
try {
+ XAttrPermissionFilter.checkPermissionForApi(pc, xAttr);
+ } catch (AccessControlException e) {
+ logAuditEvent(false, "setXAttr", src);
+ }
+ checkOperation(OperationCategory.WRITE);
{code}
We need to rethrow exception from here right insted of continueing?
{code}
void removeXAttr(String src, XAttr xAttr) throws IOException {
+ configFlag.checkXAttrsConfigFlag();
+ HdfsFileStatus resultingStat = null;
+ FSPermissionChecker pc = getPermissionChecker();
+ try {
+ XAttrPermissionFilter.checkPermissionForApi(pc, xAttr);
+ } catch (AccessControlException e) {
+ logAuditEvent(false, "removeXAttr", src);
+ }
{code}
same as above comment.
{code}
try {
+ XAttrPermissionFilter.checkPermissionForApi(pc, xAttr);
+ } catch (AccessControlException e) {
+ logAuditEvent(false, "setXAttr", src);
+ }
+ checkOperation(OperationCategory.WRITE);
+ byte[][] pathComponents =
FSDirectory.getPathComponentsForReservedPath(src);
+ writeLock();
+ try {
+ checkOperation(OperationCategory.WRITE);
+ checkNameNodeSafeMode("Cannot set XAttr on " + src);
+ src = FSDirectory.resolvePath(src, pathComponents, dir);
+ if (isPermissionEnabled) {
+ checkPathAccess(pc, src, FsAction.WRITE);
+ }
{code}
Here checkPathAccess also throws AccessControlException, so we are not
considering it for failure audit log?
I will continue my further review tomorrow on this. Thanks for the work on this
patch, Yi!
> Namenode server-side storage for XAttrs
> ---------------------------------------
>
> Key: HDFS-6258
> URL: https://issues.apache.org/jira/browse/HDFS-6258
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Affects Versions: HDFS XAttrs (HDFS-2006)
> Reporter: Yi Liu
> Assignee: Yi Liu
> Attachments: HDFS-6258.1.patch, HDFS-6258.2.patch, HDFS-6258.3.patch,
> HDFS-6258.4.patch, HDFS-6258.patch
>
>
> Namenode Server-side storage for XAttrs: FSNamesystem and friends.
> Refine XAttrConfigFlag and AclConfigFlag to ConfigFlag.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
