[
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<std::string, ConfigData> 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<hdfs::HdfsConfiguration> config =
loader.LoadDefaultResources<hdfs::HdfsConfiguration>();
{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<hbase::Configuration> config =
loader.LoadDefaultResources<hbase::Configuration>();
{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)
