[
https://issues.apache.org/jira/browse/HDFS-6258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13987464#comment-13987464
]
Yi Liu commented on HDFS-6258:
------------------------------
Thanks Uma for your review.
{quote}
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.
{quote}
Right, since the patch doesn't include persisting, so this patch will not
contain Restarting NN/saving checkpoint. Let me remove the description about
Restarting NN and Saving Checkpoint.
{quote}
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?
...
Same as above. there are many cases like that please check.
{quote}
Good point, we can use a Assert.fail after {{setXAttr}} statement.
{quote}
NNConf.java:
...
Please provide separate doc instead of combined one.
...
The second method missing javadoc.
{quote}
Right, let's update the javadoc.
{quote}
{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?
{quote}
OK, let's add the namespace info in the exception message.
{quote}
Do we have a test covering this max limit?
{quote}
Good point, let's update the test case to add a max limit check.
{quote}
Also It may be good to add the tests for checkNameNodeSafeMode here? Sorry for
missing this suggestion in my previous feedback. Ralized now.
{quote}
checkNameNodeSafeMode test will be in another JIRA, it includes restarting
NN/saving checkpoint.
{quote}
With the current code, in which case xAttrs can be null?
{quote}
OK. Let's add an assertion here.
{quote}
Since you don't use newXattrs var right now, you could have just called API to
avoid java warning.
{quote}
OK. Let's remove the warning.
{quote}
{code}
@Rule
public ExpectedException exception = ExpectedException.none();
{code}
What is the use of this Rule declaration in current tests?
{quote}
Right, it's not used now. Let's remove it.
> 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)
