hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Uma Maheswara Rao G (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HDFS-6422) getfattr in CLI doesn't throw exception or return non-0 return code when xattr doesn't exist
Date Sat, 19 Jul 2014 04:57:39 GMT

    [ 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)

Mime
View raw message