[jira] [Commented] (HADOOP-11996) Native part for ISA-L erasure coder

["Kai Zheng (JIRA)" <ji...@apache.org>]

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

Kai Zheng commented on HADOOP-11996:
------------------------------------

Thanks [~cmccabe] for the thorough and insightful review!
bq. Any globals in header files should be defined with the extern keyword, to indicate that they are declarations, not definitions. ...
This is a very good educational information for me and will also make the codes more professional. Thanks for your time helping explain this!
bq. So pick a .c file to define this global in (probably isal_load.c?) and make the header file use extern...
Yes, I will make the change, exactly.
bq. In general, we don't check in JNI-generated header files. Given that this is a pre-existing problem, we could fix this in a follow-on JIRA, though.
Agree, I can make the desired change in HADOOP-11540 where it can generate the JNI header files according to the Java class files.
bq. Is there any reason for having it when that function already exists?
Yes the duplication was introduced accidentally and can be avoided, though it's small.
bq. Is there a reason to keep coder_util.c and erasure_coder.c ...  Similarly, should the contents of erasure_code.c also be merged into coder.c? If not, why not?
Ah yeah I have to explain and also make some changes accordingly to make the intention clear. The codes are organized in two minor layers, the first wrapping the ISA-L library and building the main logics for encoding/decoding, and the second is very thin JNI part for the Java coders. Why I would prefer to have the two parts separate? Actually in my initial codes they're all mixed together, then I found it's very hard to debug and fix the logic, from Java to the native stack. Then I separated the main logic things out of the JNI environment, resulting in erasure_code.c, erasure_coder.c and etc, and wrote sample test program separately, run it, debug it and fix it. It made me much easy and lightweight, because I don't have to do the cycle in the Hadoop native building process, which is time consuming. In this way, you can also note that the test program is a very simple one, not any JNI or JVM coupled. So to make this intention clear, I will rename the files so all the JNI related files will be prefixed with {{jni_}}. Hope this also works for you. 
bq. The distinction between "util" and "coder" seems artificial to me.
I agree. I will rename {{coder_util.c}} to {{jni_common.c}}, because the functions contained in it will be shared by at least two coders, RS coder and later XOR coder.
bq. The declarations for the dump.c functions are not in dump.h as expected. Instead, they seem to be hiding in erasure_coder.h-- that was unexpected.
Yeah, good catch. I will correct it.
bq. I don't think there is a good reason to have an include directory.
I agree right now it's not so necessary as the codes introduced in the folder isn't large at all, only 20 files or so.

> Native part for ISA-L erasure coder
> -----------------------------------
>
>                 Key: HADOOP-11996
>                 URL: https://issues.apache.org/jira/browse/HADOOP-11996
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: io
>            Reporter: Kai Zheng
>            Assignee: Kai Zheng
>         Attachments: HADOOP-11996-initial.patch, HADOOP-11996-v2.patch, HADOOP-11996-v3.patch, HADOOP-11996-v4.patch, HADOOP-11996-v5.patch, HADOOP-11996-v6.patch, HADOOP-11996-v7.patch, HADOOP-11996-v8.patch
>
>
> While working on HADOOP-11540 and etc., it was found useful to write the basic facilities based on Intel ISA-L library separately from JNI stuff. It's also easy to debug and troubleshooting, as no JNI or Java stuffs are involved.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)