[
https://issues.apache.org/jira/browse/HDFS-6258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13981301#comment-13981301
]
Uma Maheswara Rao G commented on HDFS-6258:
-------------------------------------------
I think the patch is straightforward to review as a whole but I am not against
to split into small JIRAs if needed. :-)
Much of the code goes into PB related as well. so, separating that protocol
things with core logics would be good thing to do for more focus on core logic.
But I don't see a strong reason for splitting persistence and implementations
here, as we have just 3 APIs implemented to support and much of the code
already simplified as we just as INodeFature.
BTW, Here is my early review comments on the patch attached.
FsNameSystem and ClientProtocol:
- 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 retun from
clinet 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)
XAttr.java :
- {noformat}
/**
* XAttr is POSIX Extended Attribute model, similar to the one in traditional
Operating Systems.
* Extended Attribute consists of a name and associated data, and 4 namespaces
are defined: user,
* trusted, security and system.
* 1). USER namespace extended attribute may be assigned for storing
arbitrary additional
* information, and its access permissions are defined by file/directory
permission bits.
* 2). TRUSTED namespace extended attribute are visible and accessible only
to privilege user
* (file/directory owner or fs admin), and it is available from both user
space (filesystem
* API) and fs kernel.
* 3). SYSTEM namespace extended attribute is used by fs kernel to store
system objects,
* and only available in fs kernel. It's not visible to users.
* 4). SECURITY namespace extended attribute is used by fs kernel for
security features, and
* it's not visible to users.
* <p/>
* @see <a
href="http://en.wikipedia.org/wiki/Extended_file_attributes";>http://en.wikipedia.org/wiki/Extended_file_attributes</a>
*
*/
{noformat}
Please use appropriate line breaks in the java doc.
And I don't think . is necessary after each point number
- {code}
if (name == null) {
+ if (other.name != null) {
+ return false;
+ }
+ } else if (!name.equals(other.name)) {
+ return false;
+ }
{code}
Commons StringUtils does the null safe comparision. SO, can we use
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
{code}
else {
+ throw new IllegalArgumentException("XAttr name must be prefixed with
user/trusted/security/system which followed by '.'");
+ }
{code}
Same as above.
- Seems like we allow empty names ex: "user." ?
- {code}
xAttrMap.put(name, xAttr.getValue());
{code}
Seems like we are allowing null xattr values? I did not see a validation for
not having it. If so, while getting we may hit null pointer here I think.
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.
General:
- I don't see any failure audit logs
- 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.
XAttrConfigFlag.java:
I don't have strong feeliong to have a separate class for config parameter and
for a small check.
Tests:
- Can the tests in TestXAttr and TestXAttrs into one class?
- I don't see any javadoc for 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 loosing 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.
Yes, I agree with others that we should have a separate JIRA for covering XML
based test cases for command line usage. Much of the cli cases can cover there
in tests.
I remember Liu also has a thought on filing JIRA for it once the core part is
in.
If you want you can take out hdfs.java into separate jira and you can add
tests for supporting this via FileContext APIs.
> 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)
