hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Clampffer (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-10787) libhdfs++: hdfs_configuration and configuration_loader should be accessible from our public API
Date Tue, 15 Aug 2017 15:42:00 GMT

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

James Clampffer commented on HDFS-10787:
----------------------------------------

Thanks for the initial patch [~mtracy] and the options loading extention [~anatoli.shein].

Here's some feedback:

-This is a little problematic because it doesn't have a way of signalling an error.  For example
the way it's used in tools if the default path has invalid xml what should the result Options
object look like?  Can you make this look like the other methods that take a reference to
assign to and return a success flag?  That way we have a way of indicating failure but don't
have to use std::tr2::optional in the public api.
{code}
Class ConfigParser {
  ...
  Options get_options() const;
{code}

-Can you add a load_defaults method to ConfigParser rather than implicitly doing parsing files
it in the default constructor?  There's places we assume that the Options object will get
initialized to it's default values and doing this implicitly introduces dependencies on the
environment in a way that's not obvious.  Generally you want the default constructor to be
inexpensive because it's going to get called when std containers are reserving memory; "vector<Options>
foo; foo.resize(10);" will go parse the configs 10 times right now.

-Nit:  Comments about what headers are being used for often don't age well unless people spend
a lot of time looking to see if stuff is still there.  There's tools to prune out unused headers
that we should most likely get into the build.
{code}
#include <numeric>      // std::accumulate
{code}

-Might be worth adding a sanity check with an upper bound for the memory being reserved for
the xml document here.
{code}
  std::vector<char> raw_bytes((int64_t)length + 1);
{code}



> libhdfs++: hdfs_configuration and configuration_loader should be accessible from our
public API
> -----------------------------------------------------------------------------------------------
>
>                 Key: HDFS-10787
>                 URL: https://issues.apache.org/jira/browse/HDFS-10787
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: hdfs-client
>            Reporter: Anatoli Shein
>            Assignee: James Clampffer
>         Attachments: HDFS-10787.HDFS-8707.000.patch, HDFS-10787.HDFS-8707.001.patch
>
>
> Currently, libhdfspp examples and tools all have this:
> #include "hdfspp/hdfspp.h"
> #include "common/hdfs_configuration.h"
> #include "common/configuration_loader.h"
> This is done in order to read configs and connect. We want  hdfs_configuration and configuration_loader
to be accessible just by including our hdfspp.h. One way to achieve that would be to create
a builder, and would include the above libraries.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
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