[
https://issues.apache.org/jira/browse/HDFS-8766?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14975188#comment-14975188
]
James Clampffer commented on HDFS-8766:
---------------------------------------
Thanks for the input Haohui!
I have the bulk of the next patch done incorporating your feedback but it
doesn't look like I'll have it done and tested until tomorrow morning. In the
meantime I have a couple questions/comments so we can save a round trip of
reviewing if you catch this before then:
"The comments for the include files are unnecessary. The order of include files
should be (1) local headers, (2) headers that are specific to the projects and
(3) C++ headers. It avoid unnecessary loading."
Just to check: google's coding style says 1) include the header your
implementing 2) C/C++ headers 3) project files, do we want to stick with google
here? I have no preference I'd just like to make sure we're both on the same
page.
"There is a race-condition here. You're effectively hitting
https://github.com/haohui/libhdfspp/issues/31. You'll need to capture the
promise into the closure of the handler. And please wrap them as a template to
avoid copying code."
a) We should actually be safe in this case. Looking back at the old code I
noticed it wasn't really a race condition so much as a dangling reference issue
that tended to show up during races. The code that caused this was "return
stat.get_future.get()". The promise, stat, was stack allocated so when you
call get_future you get back an rvalue that holds a reference to data contained
in (or otherwise managed by) the stack allocated object. Because get is being
called on rvalue nothing was left to keep the original stat object alive as the
scope ended so in some cases stat's destructor would get called, or the stack
was unwound and rewound past that address, before the future's get method
returned. It would be similar to doing "stringstream ss;....; return
ss.str().c_str()". In this case it is guaranteed that the promise object stays
alive for the duration of the call.
b) It turns out that creating one synchronization template is tricky due to a
compiler bug in GCC. The only way I can think of doing this correctly with a
single function involves using a variadic capture list to create a wrapper
callback (to set the future) and then calling the real callback from there.
GCC doesn't seem to handle unpacking variadic arguments in capture lists in a
lot of versions (mine included). I'm going to put a little more time into
finding an alternative implementation but it looks like I may have to create 1
template for each number of arguments; it's better than nothing and portable.
If you happen to have some time to think about this and come up with something
that only needs one templated call I'd be happy to test it on GCC and use it.
Now that the FileHandle and FileSystem method implementations are separated
from the C API wrappers I think it makes sense to put that all of the C++ into
a namespace to keep things clean. Do you have any preference about namespace
nesting/naming conventions here? I'm thinking hdfs::c_bindings should suffice.
> Implement a libhdfs(3) compatible API
> -------------------------------------
>
> Key: HDFS-8766
> URL: https://issues.apache.org/jira/browse/HDFS-8766
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: James Clampffer
> Assignee: James Clampffer
> Attachments: HDFS-8766.HDFS-8707.000.patch,
> HDFS-8766.HDFS-8707.001.patch, HDFS-8766.HDFS-8707.002.patch,
> HDFS-8766.HDFS-8707.003.patch, HDFS-8766.HDFS-8707.004.patch,
> HDFS-8766.HDFS-8707.005.patch, HDFS-8766.HDFS-8707.006.patch,
> HDFS-8766.HDFS-8707.007.patch, HDFS-8766.HDFS-8707.008.patch,
> HDFS-8766.HDFS-8707.009.patch
>
>
> Add a synchronous API that is compatible with the hdfs.h header used in
> libhdfs and libhdfs3. This will make it possible for projects using
> libhdfs/libhdfs3 to relink against libhdfspp with minimal changes.
> This also provides a pure C interface that can be linked against projects
> that aren't built in C++11 mode for various reasons but use the same
> compiler. It also allows many other programming languages to access
> libhdfspp through builtin FFI interfaces.
> The libhdfs API is very similar to the posix file API which makes it easier
> for programs built using posix filesystem calls to be modified to access HDFS.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
