[
https://issues.apache.org/jira/browse/HDFS-9117?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15002991#comment-15002991
]
Haohui Mai edited comment on HDFS-9117 at 11/12/15 10:35 PM:
-------------------------------------------------------------
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}
{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.
was (Author: wheat9):
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)
