[
https://issues.apache.org/jira/browse/HDFS-7018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14252541#comment-14252541
]
Colin Patrick McCabe edited comment on HDFS-7018 at 12/18/14 11:31 PM:
-----------------------------------------------------------------------
Please don't make another copy of {{hdfs.h}} in the source tree. This will
lead to the various copies getting out of sync over time, which would be very
bad. Instead, just reference the existing copy via a relative path.
You can add your {{hdfsGetLastError}} function to this file, and just have a
dummy implementation that returns "unknown error" in all cases for
{{libwebhdfs}} and {{libhdfs}}. We can improve this in a follow-on JIRA.
{code}
39 #ifdef __cplusplus
40 extern "C" {
41 #endif
{code}
While this is needed in {{hdfs.h}}, it is not needed in your {{Hdfs.cc}} code.
The C\+\+ linker is smart enough to figure out that the prototypes it is seeing
correspond to the prototypes in the {{hdfs.h}} file you included.
{code}
45 static THREAD_LOCAL const char *ErrorMessage = NULL;
46 static THREAD_LOCAL std::string *ErrorMessageBuffer = NULL;
47 static THREAD_LOCAL hdfs::internal::once_flag once;
48
49 static void CreateMessageBuffer() {
50 ErrorMessageBuffer = new std::string;
51 }
{code}
I don't think we need all this. Making the thread-local buffer a pointer to a
{{std::string}} means that we have to check {{once_flag}} before we access it,
which is inefficient. It also means that if the thread exits, this memory will
be leaked (unless you set up a POSIX "thread destructor," which is complicated
and platform-specific).
Instead, let's just have a char\[128\] buffer for each thread. As an added
bonus, because this utilitizes pre-allocation, it handles the case where you
can't allocate memory for the error string itself, which you have said in the
past that you care about.
{code}
158 private:
159 bool input;
160 void *stream;
161 };
{code}
Please don't use {{void*}} here. It is not typesafe.
You can clearly see that FS objects and file objects have a concrete type,
spelled out in {{hdfs.h}}:
{code}
struct hdfs_internal;
typedef struct hdfs_internal* hdfsFS;
struct hdfsFile_internal;
typedef struct hdfsFile_internal* hdfsFile;
{code}
All of these functions need to have a {{catch (...)}} which sets the error
message to "unknown" and returns {{EINTERNAL}}. The reason is that if you
attempt to throw a C\+\+ exception through a C API, the program will abort
(technically, {{std::terminate}} will be called). I realize you probably think
you have caught all possible exceptions, but since this is C\+\+, we can never
really be sure without the {{catch (...)}}
P.S. thanks for working on this!
was (Author: cmccabe):
Please don't make another copy of {{hdfs.h}} in the source tree. This will
lead to the various copies getting out of sync over time, which would be very
bad. Instead, just reference the existing copy via a relative path.
You can add your {{hdfsGetLastError}} function to this file, and just have a
dummy implementation that returns "unknown error" in all cases for
{{libwebhdfs}} and {{libhdfs}}. We can improve this in a follow-on JIRA.
{code}
39 #ifdef __cplusplus
40 extern "C" {
41 #endif
{code}
While this is needed in {{hdfs.h}}, it is not needed in your {{Hdfs.cc}} code.
The C\+\+ linker is smart enough to figure out that the prototypes it is seeing
correspond to the prototypes in the {{hdfs.h}} file you included.
{code}
45 static THREAD_LOCAL const char *ErrorMessage = NULL;
46 static THREAD_LOCAL std::string *ErrorMessageBuffer = NULL;
47 static THREAD_LOCAL hdfs::internal::once_flag once;
48
49 static void CreateMessageBuffer() {
50 ErrorMessageBuffer = new std::string;
51 }
{code}
I don't think we need all this. Making the thread-local buffer a pointer to a
{{std::string}} means that we have to check {{once_flag}} before we access it,
which is inefficient. It also means that if the thread exits, this memory will
be leaked (unless you set up a POSIX "thread destructor," which is complicated
and platform-specific).
Instead, let's just have a char\[128\] buffer for each thread. As an added
bonus, because this utilitizes pre-allocation, it handles the case where you
can't allocate memory for the error string itself, which you have said in the
past that you care about.
{code}
158 private:
159 bool input;
160 void *stream;
161 };
{code}
Please don't use {{void*}} here. It is not typesafe.
You can clearly see that FS objects and file objects have a concrete type,
spelled out in {{hdfs.h}}:
{code}
struct hdfs_internal;
typedef struct hdfs_internal* hdfsFS;
struct hdfsFile_internal;
typedef struct hdfsFile_internal* hdfsFile;
{code}
All of these functions need to have a {{catch (...)}} which sets the error
message to "unknown" and returns {{EINTERNAL}}. The reason is that if you
attempt to throw a C\+\+ exception through a C API, the program will abort
(technically, {{std::terminate}} will be called). I realize you probably think
you have caught all possible exceptions, but since this is C\+\+, we can never
really be sure without the {{catch (...)}}
> Implement C interface for libhdfs3
> ----------------------------------
>
> Key: HDFS-7018
> URL: https://issues.apache.org/jira/browse/HDFS-7018
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Zhanwei Wang
> Assignee: Zhanwei Wang
> Attachments: HDFS-7018-pnative.002.patch, HDFS-7018.patch
>
>
> Implement C interface for libhdfs3
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
