[jira] [Commented] (HADOOP-11996) Native erasure coder facilities based on ISA-L

["Colin Patrick McCabe (JIRA)" <ji...@apache.org>]

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

Colin Patrick McCabe commented on HADOOP-11996:
-----------------------------------------------

Thanks for posting this, [~drankye].

{code}
/**                                                                                            
 * This is a sample program illustrating how to use the Intel ISA-L library.                   
 * Note it's adapted from erasure_code_test.c test program, but trying to use                  
 * variable names and styles we're more familiar with already similar to Java                  
 * coders.                                                                                     
 */                                                                                            
{code}
Hmm.  Why duplicate this file?  Should we just replace the old test with the new, if it is better?  Are there things that only one test or the other tests?

{code}
  load_erasurecode_lib(errMsg, sizeof(errMsg));
  if (strlen(errMsg) > 0) {
    // TODO: this may indicate s severe error instead, failing the test.
    fprintf(stderr,
      "Loading erasurecode library failed: %s, skipping this\n", errMsg);
    return 0;
  } else {
    fprintf(stdout,
      "Will run this test using %s\n", get_library_name());
  }
{code}

I don't think we should be returning 0 on failure... 0 is {{EXIT_SUCCESS}}, which means the test succeeded.

nit: the fprintf does not need to be inside an "else" block.

{code}
dataUnits = (unsigned char**)malloc(sizeof(unsigned char*) * numDataUnits);
{code}
The typecast is not needed, since {{malloc}} returns a void*

{code}
    dataUnits[i] = malloc(chunkSize);
    for (j = 0; j < chunkSize; j++) {
      dataUnits[i][j] = rand();
    }
{code}
Seems easier just to use {{calloc}}.  Same comment below.

{code}
initCoder((CoderState*)pCoderState, numDataUnits, numParityUnits);
{code}
It is safer to avoid typecasting here.  We don't need to typecast {{EncoderState}} to {{CoderState}}... can just pass {{&pCoderState.coderState}}, like this:
{code}
initCoder(&pCoderState.coderState, numDataUnits, numParityUnits);
{code}

> Native erasure coder facilities based on ISA-L
> ----------------------------------------------
>
>                 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
>
>
> 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)