[
https://issues.apache.org/jira/browse/HDFS-10754?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15435697#comment-15435697
]
Anatoli Shein edited comment on HDFS-10754 at 8/24/16 9:22 PM:
---------------------------------------------------------------
Thanks for the review, [~bobhansen].
I have addressed your comments as follows:
cat:
* Why are we including "google/protobuf/stubs/common.h?
(i) Because we need it to call ShutdownProtobufLibrary() (clean up for
protobuf).
* Comment says "wrapping fs in unique_ptr", but then we wrap it in a shared_ptr.
(/) Fixed.
* We shouldn't check the port on defaultFS before reporting error. The
defaultFS might point to an HA name. Just check that defaultFS isn't empty, and
report it if it is.
(/) Done.
* For simplicity, cat should just use file->Read rather than PositionRead
(/) Done.
gendirs:
* As an example, gendirs should definitely not need to use
fs/namenode_operations
(/) Fixed by adding hardcoded permissions instead of pulling the default ones
from the namenode_ops.
* It should not need to use common/* either; we should push the configuration
stuff into hdfspp/ (but perhaps that can be a different bug)
(i) Created a new jira for this: HDFS-10787
* We generally don't make values on the stack const (e.g. path, depth, fanout).
It's not wrong, just generally redundant (unless it's important they be const
for some reason)
(/) Fixed.
* Put a comment on setting the timeout referring to HDFS-10781 (and vice-versa)
(/) Comments added.
configuration_loader.cc:
* Add a comment on why we're calling setDefaultSearchPath in the ctor
(/) Added.
configuration_loader.h:
* I might make the comment something along the lines of "Creates a
configuration loader with the default search path (....). If you want to
explicitly set the entire search path, call ClearSearchPath() first"
(/) Fixed.
* Filesystem.cc: can SetOwner/SetPermission be written as a call to
::Find(recursive=true) with the SetOwner/SetPerimission implemented in the
callback? Then we wouldn't need three separate implementations of the recursion
logic
(/) Done. Recursive SetOwner and SetPermission now just use Find results to
launch their own asynchrous calls. The code between them is similar though, and
might need to be collapsed even further in the future.
* Does recursive SetOwner/SetPermissions accept globs both for the recursive
and non-recursive versions? We should be consistent. Perhaps SetOwner(explicit
filename, fast) and SetOwner(glob/recursive, slower) should be different methods
(i) Since SetOwner and SetPermission are now using Find to recurse the results,
we get wild-cards for free. To activate them we will possibly need to split all
recursive functions in two.
Tools impl:
* Make a usage() function that prints out the usage of the tool. Call it if
"--help", "-h", or a parameter problem occurs.h
(/) Added a usage() function which is called when parameter problem occurs, or
when "-h" flag is passed.
(i) GetOpt does not allow long options, so I did not add "--help" flag, just
"-h".
* Keep gendirs in the examples. I don't think we need a tool version of it.
(/) hdfs_gendirs removed.
* Include a comment on HDFS-9539 to fix up the tools and examples as part of
the scope.
(/) Done.
* hdfsNewBuilderFromDirectory (in hdfs.cc) should call ClearSearchPath rather
than inheriting the default. Are there any other instances in our codebase
where we're currently constructing loaders whose behavior we need to
double-check?
(/) Done. Also fixed in files: configuration_test.cc, configuration_test.h, and
hdfs_configuration_test.cc.
was (Author: anatoli.shein):
Thanks for the review, [~bobhansen].
I have addressed your comments as follows:
cat:
* Why are we including "google/protobuf/stubs/common.h?
(i) Because we need it to call ShutdownProtobufLibrary() (clean up for
protobuf).
* Comment says "wrapping fs in unique_ptr", but then we wrap it in a shared_ptr.
(/) Fixed.
* We shouldn't check the port on defaultFS before reporting error. The
defaultFS might point to an HA name. Just check that defaultFS isn't empty, and
report it if it is.
(/) Done.
* For simplicity, cat should just use file->Read rather than PositionRead
(/) Done.
gendirs:
* As an example, gendirs should definitely not need to use
fs/namenode_operations
(/) Fixed by adding hardcoded permissions instead of pulling the default ones
from the namenode_ops.
* It should not need to use common/* either; we should push the configuration
stuff into hdfspp/ (but perhaps that can be a different bug)
(i) Created a new jira for this: HDFS-10787
* We generally don't make values on the stack const (e.g. path, depth, fanout).
It's not wrong, just generally redundant (unless it's important they be const
for some reason)
(/) Fixed.
* Put a comment on setting the timeout referring to HDFS-10781 (and vice-versa)
(/) Comments added.
configuration_loader.cc:
* Add a comment on why we're calling setDefaultSearchPath in the ctor
(/) Added.
configuration_loader.h:
* I might make the comment something along the lines of "Creates a
configuration loader with the default search path (....). If you want to
explicitly set the entire search path, call ClearSearchPath() first"
(/) Fixed.
* Filesystem.cc: can SetOwner/SetPermission be written as a call to
::Find(recursive=true) with the SetOwner/SetPerimission implemented in the
callback? Then we wouldn't need three separate implementations of the recursion
logic
(/) Done. Recursive SetOwner and SetPermission now just use Find results to
launch their own asynchrous calls. The code between them is similar though, and
might need to be collapsed even further in the future.
* Does recursive SetOwner/SetPermissions accept globs both for the recursive
and non-recursive versions? We should be consistent. Perhaps SetOwner(explicit
filename, fast) and SetOwner(glob/recursive, slower) should be different methods
(i) Since SetOwner and SetPermission are now using Find to recurse the results,
we get wild-cards for free. To activate them we will possibly need to split all
recursive functions in two.
Tools impl:
* Make a usage() function that prints out the usage of the tool. Call it if
"--help", "-h", or a parameter problem occurs.h
(/) Added a usage() function which is called when parameter problem occurs, or
when "-h" flag is passed.
(i) GetOpt does not allow long options, so I did not add "--help" flag, just
"-h".
* Keep gendirs in the examples. I don't think we need a tool version of it.
(/) hdfs_gendirs removed.
* Include a comment on HDFS-9539 to fix up the tools and examples as part of
the scope.
(/) Done.
* hdfsNewBuilderFromDirectory (in hdfs.cc) should call ClearSearchPath rather
than inheriting the default. Are there any other instances in our codebase
where we're currently constructing loaders whose behavior we need to
double-check?
(/) Done. Also fixed in files: configuration_test.cc, configuration_test.h, and
hdfs_configuration_test.cc.
> libhdfs++: Create tools directory and implement hdfs_cat, hdfs_chgrp,
> hdfs_chown, hdfs_chmod and hdfs_find.
> -----------------------------------------------------------------------------------------------------------
>
> Key: HDFS-10754
> URL: https://issues.apache.org/jira/browse/HDFS-10754
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Anatoli Shein
> Assignee: Anatoli Shein
> Attachments: HDFS-10754.HDFS-8707.000.patch,
> HDFS-10754.HDFS-8707.001.patch, HDFS-10754.HDFS-8707.002.patch,
> HDFS-10754.HDFS-8707.003.patch, HDFS-10754.HDFS-8707.004.patch,
> HDFS-10754.HDFS-8707.005.patch, HDFS-10754.HDFS-8707.006.patch,
> HDFS-10754.HDFS-8707.007.patch, HDFS-10754.HDFS-8707.008.patch,
> HDFS-10754.HDFS-8707.009.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
