[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-24 Thread Sudeep Sunthankar (JIRA) 

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

Sudeep Sunthankar commented on HBASE-16489:
---

Thanks [~enis],

{quote}
This still assumes that the hbase source code is at /usr/src/hbase. Instead you 
should use relative directories or find PWD and use $PWD/build or something.
{quote}.
I will use relative paths instead. 
 
{quote}
 The best way would be to add an assertion after the config parsing which 
checks the conf optional is set.
{quote}
I will add the asserts in the subsequent patch and remove the 'if' conditions.

{quote}
Our precommit script (hadoopqa) checks for that for regular patches, but it is 
not hooked up for this branch and C++ code yet.
{quote}
Is there a standalone script which I can use to check ?

{quote}
Also forgot to mention earlier that in java code base, we have a strict line 
wrapping of 100 columns. Let's follow that in the C++ code base as well.
{quote}
I will change the line wrapping for all subsequent patches.

Thanks.

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch, HBASE-16489.HBASE-14850.v5.patch, 
> HBASE-16489.HBASE-14850.v6.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-23 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

Thanks for the latest version of the patch. A couple of comments: 
- This still assumes that the hbase source code is at {{/usr/src/hbase}}. 
Instead you should use relative directories or find PWD and use {{$PWD/build}} 
or something. 
{code}
+const std::string kHBaseConfPath("/usr/src/hbase/hbase-native-client/build/");
{code}

Same thing with kDefHBaseConfPath as well. You can manually create a conf dir 
under build if you want to set a search path there.  
- Lets rename the files as well {{configuration_loader.h}} -> 
{{hbase_configuration_loader.h}} and same for {{.cc}}. 
- You should not do these kind of conditionals in unit tests: 
{code}
+  if (conf) {
+EXPECT_STREQ((*conf).Get("custom-prop", "").c_str(), "custom-value");
+EXPECT_STRNE((*conf).Get("custom-prop", "").c_str(), "some-value");
+  }
{code} 
The goal of the unit test is to fail with an exception if there is something 
wrong, and do assertions in the success code paths. In case configuration will 
not be parsed above (let's say a later patch breaks this path), we actually 
want to fail the test so that we can catch the issue. The above code instead 
will silently succeed, thus defeating the purpose of having a unit test. The 
best way would be to add an assertion after the config parsing which checks the 
conf optional is set. Something like this: 
{code}
+  HBaseConfigurationLoader loader;
+  std::vector resources { kHBaseSiteXml };
+  hbase::optional conf = loader.LoadResources(kHBaseConfPath,
+ resources);
+ASSERT_TRUE(conf.has_value()) << "Configuration parsing failed!";
+EXPECT_STREQ((*conf).Get("custom-prop", "").c_str(), "custom-value");
+EXPECT_STRNE((*conf).Get("custom-prop", "").c_str(), "some-value");
{code}

- Also forgot to mention earlier that in java code base, we have a strict line 
wrapping of 100 columns. Let's follow that in the C++ code base as well. Our 
precommit script (hadoopqa) checks for that for regular patches, but it is not 
hooked up for this branch and C++ code yet. So we can do the manual check for 
now. 




> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch, HBASE-16489.HBASE-14850.v5.patch, 
> HBASE-16489.HBASE-14850.v6.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-19 Thread Sudeep Sunthankar (JIRA) 

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

Sudeep Sunthankar commented on HBASE-16489:
---

{quote}
Ok, but we cannot even assume that the machine running the unit test will have 
/etc/hbase/conf directory created. Better to confine everything under temp 
directories in build/test-data. 
{quote}

Our unit tests don't refer /etc/hbase/conf anywhere. It is only used as a 
default search path by the library to load hbase-default.xml or hbase-site.xml.

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch, HBASE-16489.HBASE-14850.v5.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-17 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

bq. The build/ directory is a temporary directory where we store object files, 
shared libs and other binaries on compilation. I can change the unit tests to 
create temporary xml configuration files under build/test-data
Sounds good. 
bq.  Also, We never delete from or write to /etc/hbase/conf. It is used only 
for reading.
Ok, but we cannot even assume that the machine running the unit test will have 
/etc/hbase/conf directory created. Better to confine everything under temp 
directories in build/test-data. 
bq. The idea behind using loader.Load(conf); was that we can use loader to load 
files from different search paths and use the same configuration object with 
the updated properties instead of using conf = loader.Load(); where a new 
object will be returned every time
It is fine to return a new object. In almost all of the usage, the user will be 
loding default file names from default search path. We do not need to 
complicate stuff for reloading using the same config object. Lets make it so 
that default usage is only 2 lines instead of 5 lines, similar to the HDFS 
Configuration case. 

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-17 Thread Sudeep Sunthankar (JIRA) 

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

Sudeep Sunthankar commented on HBASE-16489:
---

Thanks [~enis],

# With the help of ASSERT_NO_THROW, if there is a change in the code and 
exceptions are added for those methods or statements, tests can be added for 
those exceptions. I will remove it. If required we can add it later. 
# typedef was copied from HDFS-8707, I had planned on keeping the  'using' 
directive and remove the typedef, but forgot to do the same and ended having 
both. Sorry for that.
# The build/ directory is a temporary directory where we store object files, 
shared libs and other binaries on compilation. I can change the unit tests to 
create temporary xml configuration files under build/test-data/. Also, We never 
delete from or write to /etc/hbase/conf.  It is used only for reading. Some of 
the unit tests reset the environment variable HBASE_CONF to load properties 
from the build/ directory.
# The idea behind using loader.Load(conf); was that we can use loader to load 
files from different search paths and use the same configuration object with 
the updated properties instead of using conf = loader.Load(); where a new 
object will be returned every time. For instance, we can use loader as follows:-
{code}
Configuration conf;
HBaseConfigurationLoader loader;

// Load defaults
loader.SetDefaultSearchPath();
loader.AddDefaultResources();
// load properties
loader.Load(conf);

// load custom
loader.AddToSearchPath(CUSTOM_HBASE_CONF_PATH);
loader.AddResources(HBASE_DEFAULT_XML);
loader.AddResources(HBASE_SITE_XML);
// use the same conf object to update properties
loader.Load(conf);
{code}

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-16 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

Also for constant names, in Java we sometimes use all caps, sometimes not. I 
personally do not like all caps at all since I think it reduces readability. 
Googles style guide recommends to name all constants with a prefix of {{k}} and 
camel-case: 
https://google.github.io/styleguide/cppguide.html#Constant_Names. HDFS also 
uses this convention it seems. Let's use that going forward. We can fix the 
existing code retroactively in another issue.  

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-16 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---


 - Why do you wrap everything in  {{ASSERT_NO_THROW()}} statements? Usage of 
ASSERT_THROW() is for the case where the wrapped code will throw expected 
exceptions. However, in the case that the expectation is that the code should 
not throw exceptions, there is no need to use ASSERT_NO_THROW. If the code 
indeed throws an exception, the test method will fail anyway since it will not 
be caught and the test suite execution will fail. Please remove all such 
statements. 
 - Why are we aliasing two times below? Just pick one. Let's use the camel case 
(like {{ConfigMap}}) for similar stuff in the future as well. All caps is not 
readable. 
{code}
+  typedef std::map ConfigMap;
+  using CONFIG_MAP = Configuration::ConfigMap;
{code}

I think we have previously talked about using the Google'c C++ conventions. 
Let's use those recommendations as a guide from now on. For example 
https://google.github.io/styleguide/cppguide.html#Aliases talks about:
{code}
Don't put an alias in your public API just to save typing in the 
implementation; do so only if you intend it to be used by your clients.
{code}
Configuration is public API, but I think we are not exposing the typedefs to 
the client, so we are good there. 
Also for naming, check: 
https://google.github.io/styleguide/cppguide.html#Type_Names

 - We should not write/delete anything from unit tests for directories outside 
of the project directory. Normally all java unit tests are writing under 
{{target/}}. We can write to temporary directories under {{build/test-data/}} 
for this module., but cannot ever delete / access files from outside 
(especially not under /etc/hbase/conf). For unit testing default search path, 
you can create a tmp directory, move/write the files and set the search path 
there. 
Also you can look into moving the XMLs for the test code to be distributed / 
kept outside of the code. In maven / Java land, these kind of test resources 
will be under src/test/resources for each module. We can have test-resources or 
something and keep files there. It is not a big deal if we cannot do this 
though. 
 - Let's rename {{ConfigurationLoader}} to {{HBaseConfigurationLoader}}. 
 - Can you please follow the API that I was referring above, and also similar 
to HDFS-8707. The API that you have is: 
{code}
Configuration conf;
ConfigurationLoader loader;
loader.SetDefaultSearchPath();
loader.AddDefaultResources();
loader.Load(conf);
{code}
 
HDFS usage is something like this 
(https://github.com/apache/hadoop/blob/HDFS-8707/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/examples/cpp/cat/cat.cpp):
 
{code}
  hdfs::ConfigurationLoader loader;
  //Loading default config files core-site.xml and hdfs-site.xml from the 
config path
  hdfs::optional config = 
loader.LoadDefaultResources();
{code}

In case of HDFS, the actual configuration object knows about default and site 
files and adds those to the file names. I think it is fine for us to hard code 
hbase-default and hbase-site for now in the HBaseConfigurationLoader. The only 
thing is from user API point of view, usage should be like: 
{code}
  hbase::HBaseConfigurationLoader loader;
  //Loading default config files core-site.xml and hdfs-site.xml from the 
config path
  hbase::optional config = 
loader.LoadDefaultResources();
{code}
So, please change the Load signature to return a newly constructed 
Configuration, and also add LoadDefaultResources method. 

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch, 
> HBASE-16489.HBASE-14850.v4.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-07 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

bq. HBaseConfiguration is not aware of any SeachPaths, Resources etc. It just 
passes the arguments to ConfigurationLoader. I wanted to hide the internals of 
ConfigurationLoader from the user, so made ConfigurationLoader a friend of 
HBaseConfiguration and not exposed it to the user.
I can see that the implementation just delegates, but we want the path-related 
APIs to be in the CL instead of the Configuration object as well. The thinking 
is that, if we want to write another non-XML based way to pass configuration 
options, then another ConfigurationLoader (like for example 
PropertiesConfigurationLoader) class can be written to construct the same 
Configuration object.  

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-07 Thread Sudeep Sunthankar (JIRA) 

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

Sudeep Sunthankar commented on HBASE-16489:
---

Thanks for the feedback [~enis].

--- You are correct. std::optional is an upcoming feature in C++17, so we are 
using boost::optional. We can use std::experimental::optional, but I thought 
boost::optional would be a tried and tested API so used it instead. I will 
change it
--- Get(const std::string &key) method is clearly defined to return an empty 
string only if the property is not found. The idea was to  perform variable 
expansion only If the value returned has a size > 0, else we can return the 
default value. This method is also the basis of GetInt(), GetLong(), 
GetDouble() and GetBool(). I will change it to use optionals instead.
--- HBaseConfiguration is not aware of any SeachPaths, Resources etc. It just 
passes the arguments to ConfigurationLoader. I wanted to hide the internals of 
ConfigurationLoader from the user, so made ConfigurationLoader a friend of 
HBaseConfiguration and not exposed it to the user. 

-- Thanks

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-07 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

Thanks for making these changes. 
- std::optional is coming in C++17, but we are using 14. Is that the reason we 
are using boost::optional? What about std::experimental::optional. Seems like a 
safer bet. HDFS patch is using that instead. I tried to google which one is 
better, but did not find many references. 
{code}
using optional = std::experimental::optional;
{code}
 - This should also return an optional value, instead of returning empty value: 
+std::string HBaseConfiguration::Get(const std::string &key) const {
 - Same thing like these kind of comments: 
{code}
// raw.size() > 0 if the property is present.
{code} 
Please make it so that internally we never depend on empty strings indicating 
NULL / Not Found. In modern code, we should opt for optionals in all applicable 
places. 
 - Let's rename configuration loader to hbase configuration loader, and rename 
hbase configuration to configuration. 
 - For Configuration / ConfigurationLoader, we want to follow the interface of 
https://github.com/apache/hadoop/tree/HDFS-8707/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/lib/common.
 That means unlike the current patch, the configuration should not know 
anything about the Search paths, etc. The client API should be something like 
this: 
{code}
ConfigurationLoader loader;
loader.SetSearchPath("/foo/bar");
Configuration conf = loader.Load();
// create HBase connection from this configuration. 
{code}



> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch, HBASE-16489.HBASE-14850.v3.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-11-02 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

Thanks Sudeep for the patch. 

bq. Some of the API's have been taken from HDFS-8707 which addresses 
configuration loading
I think it is fine to have a forked version from HDFS-8707. We cannot depend on 
a not-yet-merged Hadoop native library for now. Later we can decide to see 
whether we can make use of these from libhadoop. 

- We won't support windows. We can remove the ifdefs 
{code}
if defined(WIN32)
{code}

- Why DLOG rather than LOG as used elsewhere? 
- I think we should still do the Configuration / ConfigurationLoader divide, 
and maybe have Configuration as a light-weight thing which does not know about 
search paths, XML and any other stuff, and have HBaseConfigurationLoader know 
about those. We can use the Configuration object in the Client to pass around, 
etc. However, if anybody wants to implement a non-XML based configuration 
later, then the Configuration object should be shared. 

- You cannot catch exceptions from unit tests, otherwise if a failure happens 
the test will not fail. The whole idea for unit testing is that the test will 
fail if there is an exception or unexpected condition. For this patch and 
others, we should always make it so that {{make check}} will succeed or not 
based on whether all unit tests succeed, or any unit test fails. We never rely 
on reading logs in each and every unit test run manually. 
{code}
+  } catch (const std::runtime_error &rex) {
+LOG(ERROR) << "Exception caught in HBase Configuration creation:- "
+<< rex.what();
+  }
{code}

 - Empty value for a config property is different than config property 
non-existing. Should we use optionals as in the patch for HDFS-8707? 
{code}
+  } else {
+DLOG(WARNING) << "Returning empty string as no value for[" << key << "]";
+  }
{code}

 - For client usage, we should need {{GetInt}}, {{GetLong}} and maybe 
{{GetBool}} methods at least. Others can be added later if we need those. 

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch, 
> HBASE-16489.HBASE-14850.v2.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-10-12 Thread Xiaobing Zhou (JIRA) 

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

Xiaobing Zhou commented on HBASE-16489:
---

[~sudeeps] thanks for work. HDFS-9537 and HDFS-9538 have done the work of 
loading configuration, you might want to refer to them, and reuse the work if 
possible. HDFS-9632, HDFS-9791, HDFS-10787 and HDFS-10611 are the follow up 
work or fix.

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-10-11 Thread Enis Soztutar (JIRA) 

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

Enis Soztutar commented on HBASE-16489:
---

- {{hbase-native-client/core/configuration-test.cc}} does not assert anything, 
just prints out values, so it is not a unit test. Please make sure that there 
are at least some tests. 
 - Please use a better name for this map. It is not a single property. Why are 
we typedefing this anyway? 
 +using HBASE_CONF_PROPERTY = std::map;
- This for loop is not how we do substitute variables in the java code: 
{code}
+  for (int i = 0; i < MAX_SUBSTS; i++) {
{code}
Instead of blindly iterating over the values 20 times, we do substitute 
matching at the get() time. There is a subtle difference in the case that 
Configuration is a dynamic object in Java, so the substituted variables can 
change on runtime. 
- we need a version of Get without deprecated key handling: 
{code}
+const std::string HBaseConfiguration::Get(const std::string &name,
+  const std::string &default_value) {
{code}

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Attachments: HBASE-16489.HBASE-14850.v1.patch
>
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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

[jira] [Commented] (HBASE-16489) Configuration parsing

2016-09-02 Thread Sudeep Sunthankar (JIRA) 

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

Sudeep Sunthankar commented on HBASE-16489:
---

As we are already linking Boost libraries, Imo we should use the same. Any 
thoughts

> Configuration parsing
> -
>
> Key: HBASE-16489
> URL: https://issues.apache.org/jira/browse/HBASE-16489
> Project: HBase
>  Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
>
> Reading hbase-site.xml is required to read various properties viz. 
> zookeeper-quorum, client retires etc.  We can either use Apache Xerces or 
> Boost libraries.



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