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