[
https://issues.apache.org/jira/browse/HDFS-10672?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15408376#comment-15408376
]
Anatoli Shein commented on HDFS-10672:
--------------------------------------
Thank you for the review, [~James C].
I have addressed your comments as follows:
1) In cat.c you have some nice uri parsing code. It looks like you are working
on several other file system tools that are going to be using something similar
if they have C versions. I think it'd be worth pulling out that parsing code
into it's own file so it can be shared between your tools and used by others
who need a nice C API for that. You could also write some C wrappers around the
C++ URI parsing code because it's more functionally complete if you want to go
that route. You can defer this work until some of your other utility tools
start landing; I just want to make sure we don't have several tools with the
same copy/pasted block of code.
(/) Changed it to the third-party C uri parser (uriparser2).
2) In cat.cpp you use the same C-style URI code. Since part of the value of
this work is to provide good examples of libhdfs API usage I think you should
reuse our URI parsing class from libhdfspp/lib/common/uri.h here.
(/) Changed it to the C++ URI parsing class from libhdfspp/lib/common/uri.h
3) Like Bob mentioned earlier it's worth updating this to use the
ConfigurationLoader to give you an HdfsConfiguration object that can give you
an Options object that reflects what people have in /etc/hadoop/conf or
$HADOOP_CONF_DIR. I can give you a hand here if you need some references on how
to do this (at least the way I'd do it).
(/) Options are now constructed from the ConfigurationLoader.
4) In some of the unit tests:
file_info->file_length_ = 1; //To avoid running into EOF
Best to add at least one test that makes sure we do get the EOF status when
expected. I've tried to get away with similar "small" changes to other tests
without new tests and it always ended up being more pain later on than writing
the test would have.
(/) I added a new test for this case (TestEOF) in hdfs_ext_test.c.
5) In the changes to status:
bool invalid_offset() const { return code_ == kInvalidOffset; }
Not really a blocker, but I try to follow the pattern of is_<some predicate on
object's current state> to make it really clear that what you're doing is
strictly a test even when the object isn't const qualified.
(/) Changed to is_invalid_offset()
It'd sure be nice if FileSystem::Open could just return a unique_ptr or raw
pointer IMO. Google's coding standards pretend/assert that exceptions don't
exist. For their code that might be true, but I think this is going to be a
boilerplate pattern that shows up in any code that uses libhdfs++ but is
written to use RAII in idiomatic C++. Do we want to make life easier for those
users with a more idiomatic constructor? One thing you might want to do in the
error catching block is assert that the pointer is still null (or just call
free on it).
(y) I agree, we probably should refactor this code in the future to return
unique pointers. Wrapping returned raw pointers into unique pointers is
cumbersome.
> libhdfs++: reorder directories in src/main/libhdfspp/examples, and add C++
> version of cat tool
> ----------------------------------------------------------------------------------------------
>
> Key: HDFS-10672
> URL: https://issues.apache.org/jira/browse/HDFS-10672
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Anatoli Shein
> Assignee: Anatoli Shein
> Attachments: HDFS-10672.HDFS-8707.000.patch,
> HDFS-10672.HDFS-8707.001.patch, HDFS-10672.HDFS-8707.002.patch,
> HDFS-10672.HDFS-8707.003.patch
>
>
> src/main/libhdfspp/examples should be structured like
> examples/language/utility instead of examples/utility/language for easier
> access by different developers.
> Additionally implementing C++ version of cat tool.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
