[
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.
{quote}
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.
{quote}
Sure, I will add these testcases.
{quote}I think Chris meant we could do some code sharing, maybe with a base
class for both flags.
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 APIs?
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.
{quote}
{code}
if (name == null) {
if (other.name != null) {
return false;
}
} else if (!name.equals(other.name)) {
return false;
}
{code}
{quote}
Acutually It's generated by eclipse.. The logic is not complicated, can we
keep it?
{quote}
DFSClient.java:
{code}
if (prefixIndex == -1) {
throw new IllegalArgumentException("XAttr name must be prefixed with
user/trusted/security/system which followed by '.'");
}
{code}
Good to use HadoopIllegalArgumentException
{quote}
Good point, let’s do that.
{quote}
Seems like we allow empty names ex: "user." ?
{quote}
Already fixed, but not contained in this patch.
{quote}
XAttrStorage.java:
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.
{quote}
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.
{quote}
I don't see any failure audit logs
{quote}
OK, let's add this.
{quote}
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.
{quote}
Right
{quote}
Can the tests in TestXAttr and TestXAttrs into one class?
{quote}
Sure
{quote}
I don't see any javadoc for tests
{quote}
I will add more javadoc for the tests.
{quote}
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.
{quote}
Good point, Let's add this.
{quote}
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.
{quote}
sure, let's add that.
{quote}
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.
{quote}
Agree.
> 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
(v6.2#6252)
