[
https://issues.apache.org/jira/browse/HDFS-10754?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15438333#comment-15438333
]
James Clampffer commented on HDFS-10754:
----------------------------------------
Looks good to me, +1. Thanks for taking a couple more passes. Having these
tools is great and as a bonus they give a good set of stuff to run in a
regression test suite.
{code}
//SetOwner DOES NOT guarantee that the handler will only be called once
at a time, so we DO need locking in handlerSetOwner.
{code}
Thanks for making note of this. The default of 1 worker thread has made it
very easy for race conditions to slip in.
There's some really nitpicky c++isms that aren't worth holding this up but
might be worth using in the future.
{code}
std::string port = (uri.get_port()) ? std::to_string(uri.get_port().value()) :
"";
{code}
Is a good candidate for optional::value_or to simplify things.
uri.get_port.value() is already returning a string temporary, so you don't need
to construct another rvalue, if you're doing it for clarity about the return
type then I can't complain. I haven't disassembled this specific example but
IIRC an optimized build will get rid of the second constructor anyway (not like
it's in the critical path either).
{code}
std::string port = uri.get_port().value_or("");
{code}
libhdfspp/tools/tools_common.cpp should really be
libhdfspp/tools/tools_common.cc. That'll get fixed soon enough by any other
patches that deal with tools.
{code}
SetOwnerState(const std::string & username_, const std::string & groupname_,
const std::function<void(const hdfs::Status &)> & handler_,
uint64_t request_counter_, bool find_is_done_)
: username(username_),
// cutting some stuff out
status(),
lock() {
{code}
You don't need status and lock in the initializer list; the default constructor
for member variables is called implicitly. Again, if that's more a preference
thing I'm fine with it (or maybe it's needed and I'm missing something).
> 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, HDFS-10754.HDFS-8707.010.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]
