hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Liu (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-6258) Support XAttrs from NameNode and implements XAttr APIs for DistributedFileSystem
Date Sat, 26 Apr 2014 12:05:16 GMT

    [ https://issues.apache.org/jira/browse/HDFS-6258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13981959#comment-13981959

Yi Liu commented on HDFS-6258:

Thanks Andrew, Chris and Uma for code review and good comments. 
I'm waiting for HADOOP-10520 in, so I have not updated the  patch and few comments were already
fixed in latest code. 
I will update/split the patch according to all your comments after HADOOP-10520 in.

Since we will add more test cases, the patch will become more larger. According to Chris and
Uma's suggestion, the patch will be split to:
1) Protocol definitions for XAttr and client-side implementation, and related testcases.
2) Implement the server-side storage/retrieval in memory: the inode classes, {{FSNamesystem}}
and friends. And refine {{XAttrConfigFlag}}.
3) Persistence to fsimage and edit log, and related testcases.
4) XAttr CLI test cases and other test cases.

As part of this, please make sure there are tests that cover: 1) save xattrs, restart NN,
assert xattrs reloaded from edit log, 2) save xattrs, create new checkpoint, restart NN, assert
xattrs reloaded from fsimage.
Sure, I will add these testcases.

{quote}I think Chris meant we could do some code sharing, maybe with a base class for both
Yes, what Andrew said is what I meant too. Right now, the code of {{XAttrConfigFlag}} looks
identical to {{AclConfigFlag}}, except for the specific config property and error message
strings. One idea would be to consolidate this into a single ConfigFlag class that accepts
the different properties and error message strings as constructor arguments. {quote}
OK, let's do that.

{quote}Can we keep one getXattr at {{ClientProtocol}} level instead of having more overloaded
When we validate the xattrs parameter for null and empty, we return from client side itself.
Need not pass such calls to server and validate.
This will help for reducing the one overloaded API because if the xattrs parameter is null
we treat the API like getXattrs(path) {quote}
OK, let's only keep one interface for getXAttrs in {{ClientProtocol}}. And I will add more
javadoc for this.

{quote}Please use appropriate line breaks in the java doc. {quote}
Already did, but not contained in this patch. 

if (name == null) {
  if (other.name != null) {
    return false;
} else if (!name.equals(other.name)) {
  return false;
Acutually It's generated by eclipse..  The logic is not complicated, can we keep it?

if (prefixIndex == -1) {
  throw new IllegalArgumentException("XAttr name must be prefixed with user/trusted/security/system
which followed by '.'");

Good to use HadoopIllegalArgumentException
Good point, let’s do that.

Seems like we allow empty names ex: "user." ?
Already fixed, but not contained in this patch.

    Why can't we validate this much earlier. Why do we need to carry this validation parameter
till the storage?
    I would like to see the Xattr storage format on the java doc of XattrStorage class.
The reason is: before validating we need to get whether xattr exists and we need to lock FS,
we will iterate xattrs in {{XAttrStorage}}, so we handle it in {{XAttrStorage}} then we just
iterate xattrs one time. Of course we can do this in {{FSNamesystem}} and {{FSDirectory}},
but we need to iterate more times, that's not efficient.
I will add more java doc.

I don't see any failure audit logs
OK, let's add this.

I think we may need to add the layout version in NamenodeLayoutVersion as the aplit happend
as part of RollingUpgrades I guess. Even I see ACL version number tracked there in NamenodeLayoutversion
as well. Please check this.

Can the tests in TestXAttr and TestXAttrs into one class?

I don't see any javadoc for tests
I will add more javadoc for the tests.

Some key cases to cover

    I don't see NN restart cases. Check after restart wether the NN is able to get the xattrs
which was set before restart.
    Please include some test cases for HA failover and getxattrs from other node
    See if they are losing after a checkpoint etc in HA
    And I did not see a test for validating the behaviour that no flags API is behaving same
as multi-flagged API here. But no force on this as the API is just delegation with multiflags.

Good point, Let's add this.

I have one more thing to add to my list of requested test cases. Please update {{TestSafeMode}}
to verify that attempts to set xattrs during safe mode get rejected. This is important for
confirming that {{FSNamesystem}} has the proper calls to checkNameNodeSafeMode.
sure, let's add that.

On the topic of splitting the patch, I don't feel strongly that it needs to be done exactly
the way I suggested. I would prefer to see some kind of split though. I always think I can
give a more focused review when a patch is no larger than about 50k, just as a rough guideline.
Note also that a lot of the feedback so far relates to adding more tests, so overall this
is likely to grow larger and larger as the feedback gets incorporated.

> Support XAttrs from NameNode and implements XAttr APIs for DistributedFileSystem
> --------------------------------------------------------------------------------
>                 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.patch
> This JIRA is to implement extended attributes in HDFS: support XAttrs from NameNode,
implements XAttr APIs for DistributedFileSystem and so on.

This message was sent by Atlassian JIRA

View raw message