[
https://issues.apache.org/jira/browse/HDFS-10874?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
James Clampffer updated HDFS-10874:
-----------------------------------
Attachment: HDFS-10874.HDFS-8707.002.patch
Thanks [[email protected]] and [~mdeepak] for the reviews. Uploaded the new
002 patch. Deepak looks like I just missed you're before making the patch but
I can inline comments and let me know what you think.
New:
-std::pair<string, string> is now a URI::Query object with more descriptive
key and value fields
-updated tests to use the new api, added an extra test while I was there
since URI::remove_query didn't have coverage.
Followup for Deepaks comments:
bq. src/main/native/libhdfspp/lib/common/uri.cc:142 set default *result =
nullopt;
The out variable result is now just a plain int reference so it can't be
assigned to nullopt. Part of the reason for the switch away from optional was
having to remember to do that was a little error prone.
As far as catching the default exception goes I'm hesitant for a couple reasons:
-URI::parse_from_string should only be throwing a uri_parse_error. Since it's
a leaf function (aside from the library calls it makes) it's easy to guarantee
that's the only type a caller needs to catch. Catching default, assuming you
mean the catch(...) approach, generally isn't something you want if you're in
control of the callee code. It has the potential to hide things that it
shouldn't and if the rest of the design is consistent nothing should end up
there*, it can also lead people to believe that we expect a bunch of other
types and used it as a shortcut to handle them. We could do a LogWarning or
similar if you want but those get ignored by tests. The only place the code
currently does catch(...) is around user callbacks since we can't restrict what
gets passed in.
*The URI parser library can throw a bunch of different exception types, and
generally when it does it's already screwed up some memory accesses. I'd
rather have this propagate out and have sufficient test coverage to make sure
we don't hit that in our code paths.
Let me know what you think. I'm fine with adding them but IMHO for this type
of stuff it can make the code more confusing.
> libhdfs++: Public API headers should not depend on internal implementation
> --------------------------------------------------------------------------
>
> Key: HDFS-10874
> URL: https://issues.apache.org/jira/browse/HDFS-10874
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: James Clampffer
> Assignee: James Clampffer
> Attachments: HDFS-10874.HDFS-8707.000.patch,
> HDFS-10874.HDFS-8707.001.patch, HDFS-10874.HDFS-8707.002.patch
>
>
> Public headers need to do some combination of the following: stop including
> parts of the implementation, forward declare bits of the implementation where
> absolutely needed, or pull the implementation into include/hdfspp if it's
> inseparable.
> Example:
> If you want to use the C++ API and only stick include/hdfspp in the include
> path you'll get an error when you include include/hdfspp/options.h because
> that goes and includes common/uri.h.
> Related to the work described in HDFS-10787.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
