[
https://issues.apache.org/jira/browse/HDFS-7018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14266484#comment-14266484
]
Colin Patrick McCabe commented on HDFS-7018:
--------------------------------------------
Thanks, this looks a lot better.
The CMake changes look good.
{code}
64 static inline char *Strdup(const char *str) {
65 if (str == NULL) {
66 return NULL;
67 }
68
69 int len = strlen(str);
70 char *retval = new char[len + 1];
71 memcpy(retval, str, len + 1);
72 return retval;
73 }
{code}
Please, let's just use the regular {{strdup}} provided by the system. Then we
also don't have to worry about "array delete" versus regular delete either.
{{struct hdfsFile_internal}}: we can simplify this a lot. Rather than having
setters, just have a constructor that takes an {{InputStream}}, and another one
that takes an {{OutputStream}}. We shouldn't need to alter the streams after
the {{hdfsFile_internal}} object has been created. Using a "union" here is
overly complicated, and not really saving any space. On a 64-bit machine the
boolean you need to select which type the union pads the structure out to 16
bytes anyway. Just have a pointer to an input stream, and a pointer to an
output stream, and the invariant that one of them is always {{null}}.
{code}
166 class DefaultConfig {
167 public:
168 DefaultConfig() : reportError(false) {
169 const char *env = getenv("LIBHDFS3_CONF");
170 std::string confPath = env ? env : "";
{code}
We should be looking at CLASSPATH and searching all those directories for XML
files, so that we can be compatible with the existing libhdfs code. Also,
Hadoop configurations include multiple files, not just a single file. You can
look at how I did it in the HADOOP-10388 branch, which has a working
implementation of this. Alternately we could punt this to a follow-on JIRA.
{code}
224 struct hdfsBuilder {
225 public:
226 hdfsBuilder(const Config &conf) : conf(conf), port(0) {
227 }
228
229 ~hdfsBuilder() {
230 }
231
232 public:
{code}
We don't need line 232. It's a bit confusing because I expected the line to
say "private"
{{PARAMETER_ASSERT}}: this isn't what people usually mean by an {{assert}}.
Usually an {{assert}} is something that only takes effect in debug builds, and
is used to guard against programmer mistakes. In contrast, this is validating
a parameter passed in by the library user. I would prefer not to have this
macro at all since I think we ought to actually provide a detailed error
message explaining what is wrong. This macro will just fill in something like
"invalid parameter" for EINVAL, which is not very informative. I also think
it's confusing to have "return" statements in macros... maybe we can do this
occasionally, but only for a VERY good reason or in a unit test.
{code}
519 } catch (const std::bad_alloc &e) {
520 SetErrorMessage("Out of memory");
521 errno = ENOMEM;
522 } catch (...) {
523 SetErrorMessage("Unknown Error");
524 errno = EINTERNAL;
525 }
{code}
I see this repeated a lot in the code. Why can't we use
{{CreateStatusFromException}} to figure out exactly what is wrong, and derive
the errno and error message from the Status object?
Since we're adopting the Google C\+\+ style, we will eventually remove the
throw statements from other parts of the code, and then these "outer catch
block" in the C API will be the only catch blocks left, and the only users of
{{CreateStatusFromException}}, right?
{{hdfs.h}}: it's problematic to add stuff to this file until the other
implementations support it. We could get away with returning ENOTSUP from
these functions. But I think we need to discuss what some of them do... I'm
not familiar with the "get delegation token", "free delegation token" APIs and
we need to discuss what they do and if we want them, etc. I think it's best to
file a follow-on for that and leave it out for now.
> 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-pnative.003.patch, HDFS-7018.patch
>
>
> Implement C interface for libhdfs3
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
