[
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<std::string> resources { kHBaseSiteXml };
+ hbase::optional<Configuration> 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)
