[jira] [Comment Edited] (HDFS-6422) getfattr in CLI doesn't throw exception or return non-0 return code when xattr doesn't exist

["Uma Maheswara Rao G (JIRA)" <ji...@apache.org>]

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

Uma Maheswara Rao G edited comment on HDFS-6422 at 7/19/14 4:57 AM:
--------------------------------------------------------------------

Thank you, Charles for the patch. Please find my feedback as follows.

 FSNamesystem.java:
- logAuditEvent(false, "getXAttr", src); --> logAuditEvent(false, "getXAttrs", src);

- 
{code}
} else {
+        throw new IOException("No matching attributes found");
{code}
One suggestion to tell "No matching attributes found to remove"
And this condition makes me to think on retryCache. I hope it is done here let me see there.
Example first call may succeed internally but restarted/disconnected, in that case idempotent API will be retried from client. So, next call may fail as it was already removed. Do you think, we need to mark this as ATMostOnce?
Let me know, if you agree for it, I would be happy to help in fixing that.


TestDFSShell.java:

- From the below code, we don't need out.toString as we did not asserted anything.
{code}
 {
+        final int ret = ToolRunner.run(fshell, new String[] {
+            "-setfattr", "-n", "user.a1", "-v", "1234", "/foo"});
+        assertEquals("Returned should be 0", 0, ret);
+        final String str = out.toString();
+        out.reset();
+      }
{code}


- We need to shutdown the mini cluster as well.
{code}
} finally {
+      if (bakErr != null) {
+        System.setErr(bakErr);
+      }
+    }
{code}

 FSXAttrBaseTest.java:

- Please handle only specific exceptions. If it throws unexpected exception let it throwout, we need not assert and throw.
{code}
} catch (HadoopIllegalArgumentException e) {
+    } catch (Exception e) {
{code}


- if an an xattr --> if an xattr


- Please do handle specific exceptions.
{code}
 } catch (Exception e) {
+      GenericTestUtils.assertExceptionContains
+          ("An XAttr name must be prefixed with user/trusted/security/system, " +
+           "followed by a '.'",
+          e);
+    }
+
{code}

- XattrNameParam.java:

- 
{code}
private static Domain DOMAIN = new Domain(NAME,
+      Pattern.compile(".*"));
{code}
I understand that we try to eliminate the client validation as we will not have flexibility to add more namespaces in future. But that pattern can be same as <Namespace>. right.
So, how about validating pattern? Please check with Andrew as well what he says. But I have no strong feeling on that. It is a suggestion.





was (Author: umamaheswararao):
 FSNamesystem.java:
- logAuditEvent(false, "getXAttr", src); --> logAuditEvent(false, "getXAttrs", src);

- 
{code}
} else {
+        throw new IOException("No matching attributes found");
{code}
One suggestion to tell "No matching attributes found to remove"
And this condition makes me to think on retryCache. I hope it is done here let me see there.
Example first call may succeed internally but restarted/disconnected, in that case idempotent API will be retried from client. So, next call may fail as it was already removed. Do you think, we need to mark this as ATMostOnce?
Let me know, if you agree for it, I would be happy to help in fixing that.


TestDFSShell.java:

- From the below code, we don't need out.toString as we did not asserted anything.
{code}
 {
+        final int ret = ToolRunner.run(fshell, new String[] {
+            "-setfattr", "-n", "user.a1", "-v", "1234", "/foo"});
+        assertEquals("Returned should be 0", 0, ret);
+        final String str = out.toString();
+        out.reset();
+      }
{code}


- We need to shutdown the mini cluster as well.
{code}
} finally {
+      if (bakErr != null) {
+        System.setErr(bakErr);
+      }
+    }
{code}

 FSXAttrBaseTest.java:

- Please handle only specific exceptions. If it throws unexpected exception let it throwout, we need not assert and throw.
{code}
} catch (HadoopIllegalArgumentException e) {
+    } catch (Exception e) {
{code}


- if an an xattr --> if an xattr


- Please do handle specific exceptions.
{code}
 } catch (Exception e) {
+      GenericTestUtils.assertExceptionContains
+          ("An XAttr name must be prefixed with user/trusted/security/system, " +
+           "followed by a '.'",
+          e);
+    }
+
{code}

- XattrNameParam.java:

- 
{code}
private static Domain DOMAIN = new Domain(NAME,
+      Pattern.compile(".*"));
{code}
I understand that we try to eliminate the client validation as we will not have flexibility to add more namespaces in future. But that pattern can be same as <Namespace>. right.
So, how about validating pattern? Please check with Andrew as well what he says. But I have no strong feeling on that. It is a suggestion.




> getfattr in CLI doesn't throw exception or return non-0 return code when xattr doesn't exist
> --------------------------------------------------------------------------------------------
>
>                 Key: HDFS-6422
>                 URL: https://issues.apache.org/jira/browse/HDFS-6422
>             Project: Hadoop HDFS
>          Issue Type: Bug
>    Affects Versions: 3.0.0, 2.5.0
>            Reporter: Charles Lamb
>            Assignee: Charles Lamb
>            Priority: Blocker
>         Attachments: HDFS-6422.005.patch, HDFS-6422.006.patch, HDFS-6422.1.patch, HDFS-6422.2.patch, HDFS-6422.3.patch, HDFS-6474.4.patch
>
>
> If you do
> hdfs dfs -getfattr -n user.blah /foo
> and user.blah doesn't exist, the command prints
> # file: /foo
> and a 0 return code.
> It should print an exception and return a non-0 return code instead.



--
This message was sent by Atlassian JIRA
(v6.2#6252)