[ 
https://issues.apache.org/jira/browse/HDFS-7018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14274827#comment-14274827
 ] 

Zhanwei Wang commented on HDFS-7018:
------------------------------------

Hi [~cmccabe]

bq. 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.

Yes, but it introduce more trouble. 1) {{strdup}} use {{malloc}} to allocate 
memory, then we have to worry about "free" versus regular delete. 2) it cannot 
handle NULL input, that means we have to handle NULL outside and introduce many 
duplicate code. 3) Instead of throw std::bad_alloc, we have to handle OOM when 
{{strdup}} returns NULL, and introduce more duplicate code. We expect to handle 
all OOM in one place.


bq. 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.

Let's add this feature in another JIRA.

bq. {{PARAMETER_ASSERT}}: this isn't what people usually mean by an {{assert}}. 
... I would prefer not to have this macro at all ... I also think it's 
confusing to have "return" statements in macros...

How about change its name to {{CHECK_INPUT_PARAMETER}}? We have to check the 
user input parameters in APIs, right? Macro can reduce the duplicated works 
(set error message, set errno ...). If you still have concern, it is ok to me 
to remove this macro and extend its content manually like this and provide 
informative error message.

{code}
tSize hdfsWrite(hdfsFS fs, hdfsFile file, const void *buffer, tSize length) {
    if (NULL == fs) {
        SetErrorMessage("Input parameter \"fs\" is NULL"); 
        errno = EINVAL; 
        return -1; 
    }

   if (NULL == file) {
        SetErrorMessage("Input parameter \"file\" is NULL"); 
        errno = EINVAL; 
        return -1; 
    }

   if (false == file->isInput()) {
        SetErrorMessage("Input parameter \"file\" is not writable"); 
        errno = EINVAL; 
        return -1; 
    }

   if (NULL == buffer) {
        SetErrorMessage("Input parameter \"buffer\" is NULL"); 
        errno = EINVAL; 
        return -1; 
    }

   if (length <= 0) {
        SetErrorMessage("Input parameter \"length\" should larger than 0"); 
        errno = EINVAL; 
        return -1; 
    }

...
}

{code}

bq. Since we're adopting the Google C++ style, we will eventually remove the 
throw statements from other parts of the code

I do not think we will eventually remove the throw statements. Just like Google 
said in Google C++ style. 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Exceptions

bq. On their face, the benefits of using exceptions outweigh the costs, 
especially in new projects. However, for existing code, the introduction of 
exceptions has implications on all dependent code.

The only thing the Google concerns is that the existing code cannot handle 
exceptions.

libhdfs3 is a new project and we also decided not to make the exceptions cross 
the boundary of the APIs. Existing code can work well with libhdfs3 and do not 
worry about the exceptions. So we can use exception internally to significantly 
reduce the complexity of error handling in libhdfs3. 


bq. 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?

We have used {{CreateStatusFromException}} to handle the exceptions in {{C++}} 
APIs. And in C APIs, we only need to take care one case, that is 
{{std::bad_alloc}}. Please note that {{CreateStatusFromException}} will throw 
{{std::bad_alloc}}.

As you have commented previously,  we also have to add {{catch (...)}} , since 
it is {{C++}} code. So although it repeated a lot in code, I do not think it 
can be removed.

bq. 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.

You are right. Let's discuss it in another jira to make this jira simple.







> 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)

Reply via email to