[
https://issues.apache.org/jira/browse/HDFS-6258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13986846#comment-13986846
]
Uma Maheswara Rao G commented on HDFS-6258:
-------------------------------------------
Thanks a lot Yi, for the update on the patch and addressing for all of my
feedback!
Here are my next set of comments on the latest patch!
{code}
/**
+ * Tests for creating xattr
+ * 1. create xattr using XAttrSetFlag.CREATE flag.
+ * 2. Assert exception of creating xattr which already exists.
+ * 3. Create multiple xattrs
+ * 4. Restart NN, save checkpoint scenarios.
+ */
{code}
Javadoc for tests saying Restart NN and checkpoint scenarios. But actually we
don't have tests related to that here. Of course, we can not cover then now as
we don't persist them yet.
{code}
try {
+ fs.setXAttr(path, name1, value1, EnumSet.of(XAttrSetFlag.CREATE));
+ } catch (IOException e) {
+ exceptionExpected = true;
+ }
+ Assert.assertTrue(exceptionExpected);
{code}
Instead of assert true=true, we can add fail(msg) method after setXattr and
empty catch block?. if that does not throw exception fail method will make your
test fail and provide the message in fail. So, we need not have flag?
{code}
//set xattr with empty name: "user."
+ exceptionExpected = false;
+ try {
+ fs.setXAttr(path, "user.", value1, EnumSet.of(XAttrSetFlag.CREATE,
+ XAttrSetFlag.REPLACE));
+ } catch (HadoopIllegalArgumentException e) {
+ exceptionExpected = true;
+ }
+ Assert.assertTrue(exceptionExpected);
{code}
Same as above. there are many cases like that please check.
NNConf.java:
{code}
/**
* Support for ACLs or XAttrs is controlled by a configuration flag.
* If the configuration flag is false, then the NameNode will reject
* all ACL-related or XAttr-related operations.
*/
private final boolean aclsEnabled;
private final boolean xattrsEnabled;
{code}
Please provide separate doc instead of combined one.
{code}
/**
* Checks the flag on behalf of an ACL API call.
*
* @throws AclException if ACLs are disabled
*/
public void checkAclsConfigFlag() throws AclException {
if (!aclsEnabled) {
throw new AclException(String.format(
"The ACL operation has been rejected. "
+ "Support for ACLs has been disabled by setting %s to false.",
DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY));
}
}
public void checkXAttrsConfigFlag() throws IOException {
{code}
The second method missing javadoc.
{code}
throw new AccessControlException("User doesn't have permission for xattr: "
+ + xAttr.getName());
+ }
{code}
I think the message could also tell about the passed namespace?
{code}
if (xAttrs.size() > inodeXAttrsLimit) {
+ throw new IOException("Operation fails, XAttrs of " +
+ "inode exceeds maximum limit of " + inodeXAttrsLimit);
+ }
{code}
Do we have a test covering this max limit?
Also It may be good to add the tests for checkNameNodeSafeMode here? Sorry for
missing this suggestion in my previous feedback. Ralized now.
{code}
static List<XAttr> filterXAttrsForApi(FSPermissionChecker pc,
List<XAttr> xAttrs) {
if (xAttrs == null || xAttrs.isEmpty()) {
return xAttrs;
}
{code}
With the current code, in which case xAttrs can be null?
{code}
try {
List<XAttr> newXAttrs = unprotectedRemoveXAttr(src, xAttr);
//TODO: Recording XAttrs modifications to edit log will be
//implemented as part of HDFS-6301
} finally {
writeUnlock();
}
{code}
Since you don't use newXattrs var right now, you could have just called API to
avoid java warning.
{code}
@Rule
public ExpectedException exception = ExpectedException.none();
{code}
What is the use of this Rule declaration in current tests?
> 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.5.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)
