hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bob Hansen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-10515) libhdfs++: Implement mkdirs, rmdir, rename, and remove
Date Wed, 15 Jun 2016 14:49:09 GMT

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

Bob Hansen commented on HDFS-10515:
-----------------------------------

Great work, [~anatoli.shein].  Thanks for the contribution.

For FileSystem::Delete, it is more efficient to pass a bool than a const bool &.

When passing on errors, it is valuable to provide context of where the error came from.  For
example, in FileSystemImpl::Rename:
   handler(Status::InvalidArgument("Argument 'oldPath' cannot be empty"));
could better be
   handler(Status::InvalidArgument("rename: argument 'oldPath' cannot be empty"));

In NameNodeOperations::mkdirs and NameNodeOperations::delete, if the return message has an
error, we throw away the error state and create a new PathNotFound Status.  Would it be better
to just pass the returned error Status on to the handler?  You include a comment for Rename
that the returned error is not informative; if the same is true for the others, include a
comment there.  

If there's a permissions issue, do we get a good error message back to that effect?  If so,
we should pass it back.

For the error message in NameNodeOperations::rename, perhaps "oldPath and parent directory
of newPath must exist. newPath must not exist."

We missed it in the previous reviews, but since you're touching it here... for handlers in
methods like AllowSnapshot where we're not doing any translation, we can pass the consumer
handler directly into the NameNodeOps class instead of making a lambda that just calls into
the consumer handler.

HdfsExtTest::TestMkDirs - does the java return OK if you try to createDirectory on an existent
directory?

In HdfsExtTest::TestRename, don't forget to free the results of hdfsListDirectory.


Minor nit: we don't typically add the "const" flag to parameters passed by value (as in FileSystemImpl::mkdirs).



> libhdfs++: Implement mkdirs, rmdir, rename, and remove
> ------------------------------------------------------
>
>                 Key: HDFS-10515
>                 URL: https://issues.apache.org/jira/browse/HDFS-10515
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: hdfs-client
>            Reporter: Anatoli Shein
>            Assignee: Anatoli Shein
>         Attachments: HDFS-10515.HDFS-8707.000.patch, HDFS-10515.HDFS-8707.001.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message