[
https://issues.apache.org/jira/browse/HDFS-9537?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15053497#comment-15053497
]
James Clampffer commented on HDFS-9537:
---------------------------------------
"Have thing public methods that call into a template to do the actual
Load/AddResource calls (this is what's implemented in the patch). This means a
chunk of boilerplate code to act as the public wrappers around the
implementations"
This is the way to go in my opinion as well. Your second patch doesn't have
all that much boilerplate and gives a nice simple API, the first one had quite
a bit so I was on the fence.
I've seen and commented on the majority of what this contains back in HDFS-9117
and it looks like this in line with those so I think this is a solid patch.
The only question I have is about ConfigurationLoader::OverlayResourceString
and ConfigurationLoader::UpdateMapWithString. Why you need to deep copy the
string into a char vector? Can this simply be a reinterpret_cast on
std::string::data? Based on some initial profiling it looks like we spend an
awful amount of time doing heap allocations. Clearly this isn't a critical
path but I think fewer copies are better.
The only other (incredibly pedantic) concern I have is the comment for
OverlayResourceString threw me off a bit.
"Loads Configuration XML contained in a string and adds it to the src
Configuration object, returning a new copy that is the union of the two."
If I didn't see that src was being passed by const reference I would have
assumed that the function took the source, added the new values to it, and then
returned a deep copy of the mutated source. Perhaps reword that to make it
clear that src isn't being mutated? I don't consider this a blocker, feel free
to leave as is, just something to think about.
> libhdfs++: implement HDFSConfiguration class
> --------------------------------------------
>
> Key: HDFS-9537
> URL: https://issues.apache.org/jira/browse/HDFS-9537
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Bob Hansen
> Assignee: Bob Hansen
> Attachments: HDFS-9537.HDFS-8707.000.patch,
> HDFS-9537.HDFS-8707.001.patch
>
>
> Create a class to encode the rules for interpreting a Configuration class to
> create a libhdfs++ Options object
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
