hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Haohui Mai (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-9117) Config file reader / options classes for libhdfs++
Date Thu, 12 Nov 2015 21:45:11 GMT

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

Haohui Mai commented on HDFS-9117:
----------------------------------

Thanks for splitting it up.

{code}
+bool Configuration::AddResourceStream(std::istream & stream)
+{
+  stream.seekg(0,std::ios::end);
+  std::streampos length = stream.tellg();
+  stream.seekg(0,std::ios::beg);
...
{code}

It's not streaming per se but reads the whole stream. In the first cut maybe we can remove
it from the API and keep the version that only takes string?

{code}
...
+  public:
+    // Constructs a configuration with no search path and no resources loaded
+    Configuration();
+
+    // clears out search path and all loaded values
+    void Clear();
+
+    // Loads resources from a file or a stream
+    bool AddResourceStream(std::istream & stream);
+    bool AddResourceString(const std::string & stream);
{code}

One messy problem we have in the Java side is to implement thread safety for the configuration
class. I suggest making Configuration as an immutable object, by tweaking the APIs to the
following to eliminate the problem of thread safety by design:

{code}
/**
 * A Configuration object is an immutable object that holds the configuration values specified
from the XML.
 * ...
 **/
class Configuration {
public:
  static Configuration *Load(const std::string &string);
  // This can be implement in a separate jira.
  static Configuration *LoadFromFile(...);
  // Immutable object. No add / clear methods.
};

{code}
+    std::string             GetWithDefault        (const std::string & key, const std::string
& defaultValue);
+    optional<std::string>   Get                   (const std::string & key);
+    int64_t                 GetIntWithDefault     (const std::string & key, int64_t defaultValue);
+    optional<int64_t>       GetInt                (const std::string & key)
...
{code}

Instead of having calls for every types, I suggest adopting the APIs of boost's property tree,
which exposes a single {{get}} method with a template argument:

{code}
template<class Type>
optional<Type> get(const string& key) const;
{code}

That gives a minimal exposure from the API prospective.

{code}

+    // Transparent data holder for property values
+    struct ConfigData {
+      std::string value;
+      bool        final;
+      ConfigData() : final(false) {};
+      ConfigData(const std::string &value) : value(value), final(false) {}
+      void operator = (const std::string & newValue) {value = newValue; final = false;}
+    };
+    std::map<std::string, ConfigData> raw_values;

{code}

It's preferable to move the details of on implementing {{final}} in the {{.cc}} file.

{code}
hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/configuration_test.h
{code}

Do you want to merge the {{.h}} file with the {{.cc}} file?

There are several issues on styling w.r.t. HDFS-9328, but it's okay to fix it in the next
iteration. 


> Config file reader / options classes for libhdfs++
> --------------------------------------------------
>
>                 Key: HDFS-9117
>                 URL: https://issues.apache.org/jira/browse/HDFS-9117
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: hdfs-client
>    Affects Versions: HDFS-8707
>            Reporter: Bob Hansen
>            Assignee: Bob Hansen
>         Attachments: HDFS-9117.HDFS-8707.001.patch, HDFS-9117.HDFS-8707.002.patch, HDFS-9117.HDFS-8707.003.patch,
HDFS-9117.HDFS-8707.004.patch, HDFS-9117.HDFS-8707.005.patch, HDFS-9117.HDFS-8707.006.patch,
HDFS-9117.HDFS-8707.008.patch, HDFS-9117.HDFS-8707.009.patch, HDFS-9117.HDFS-8707.010.patch,
HDFS-9117.HDFS-8707.011.patch, HDFS-9117.HDFS-8707.012.patch, HDFS-9117.HDFS-8707.013.patch,
HDFS-9117.HDFS-8707.014.patch, HDFS-9117.HDFS-9288.007.patch
>
>
> For environmental compatability with HDFS installations, libhdfs++ should be able to
read the configurations from Hadoop XML files and behave in line with the Java implementation.
> Most notably, machine names and ports should be readable from Hadoop XML configuration
files.
> Similarly, an internal Options architecture for libhdfs++ should be developed to efficiently
transport the configuration information within the system.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message