[
https://issues.apache.org/jira/browse/HDFS-10754?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15432868#comment-15432868
]
Bob Hansen commented on HDFS-10754:
-----------------------------------
This is lots of good work, [~anatoli.shein]. I have a handful of mostly minor
things I might propose:
cat:
* Why are we including "google/protobuf/stubs/common.h?
* Comment says "wrapping fs in unique_ptr", but then we wrap it in a shared_ptr.
* 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.
* For simplicity, cat should just use file->Read rather than PositionRead
gendirs:
* As an example, gendirs should _definitely_ not need to use
fs/namenode_operations
* It should not need to use common/* either; we should push the configuration
stuff into hdfspp/ (but perhaps that can be a different bug)
* Again, why include the protobuf header?
* 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)
* Put a comment on setting the timeout referring to HDFS-10781 (and vice-versa)
configuration_loader.cc:
* Add a comment on why we're calling setDefaultSearchPath in the ctor
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"
* 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
* 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
Tools impl:
* Make a usage() function that prints out the usage of the tool. Call it if
"--help", "-h", or a parameter problem occurs.
* Keen gendirs in the examples. I don't think we need a tool version of it.
Include a comment on HDFS-9539 to fix up the tools and examples as part of the
scope.
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?
> libhdfs++: Create tools directory and implement hdfs_cat, hdfs_chgrp,
> hdfs_chown, and hdfs_chmod
> ------------------------------------------------------------------------------------------------
>
> 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
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
