[jira] [Commented] (HADOOP-11887) Introduce Intel ISA-L erasure coding library for the native support

[Colin Patrick McCabe (JIRA)]

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

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

Thanks, [~drankye].

{code}
+  * Use -Disal.prefix to specify a nonstandard location for the libisal
+    library files. You do not need this option if you have installed isal.
{code}
Should be "you do not need this option if you have installed ISAL to the system 
library path."

{code}
+        ${D}/io/erasurecode/coder/erasure_code_native.c)
+
+
+endif (ISAL_LIBRARY)
{code}
Nitpick: it's weird to have two blank lines here?

{{CMakeLists.txt}}: {{REQUIRE_ISAL}} is not verified.  I was able to build 
without ISAL installed and {{-Drequire.isal}} on the maven command line, and 
there was no error.

{code}
+  static {
+    if (NativeCodeLoader.isNativeCodeLoaded() &&
+        NativeCodeLoader.buildSupportsIsal()) {
+      try {
+        loadLibrary();
+        nativeIsalLoaded = true;
+      } catch (Throwable t) {
+        LOG.error("Failed to load ISA-L", t);
+      }
+    }
+  }
+
...
+  /**
+   * Is the native ISA-L library loaded & initialized? Throw exception if not.
+   */
+  public static void checkNativeCodeLoaded() {
+    if (!nativeIsalLoaded) {
+      throw new RuntimeException("Native ISA-L library not available: " +
+          "this version of libhadoop was built without " +
+          "ISA-L support.");
+    }
+  }
{code}

Hmm.  It seems that {{checkNativeCodeLoaded}} will throw an exception stating 
that "this verison of libhadoop was built without ISA-L support" even if 
libhadoop.so *was* built with ISA-L support, but we can't find the ISA-L 
library.

Rather than having a boolean, you should have a {{loadingFailureReason}} that 
gets initialized in a static block.  Look at {{DomainSocket.java}} or 
{{OpensslAesCtrCryptoCodec.java}} to see how to do this.  Then you can throw an 
exception with this loading failure reason in that check function.

{{NativeLibraryChecker.java}}: thanks for adding support here.  It would be 
nice to distinguish between a hadoop build that doesn't support ISA-L, and one 
that can't find {{libisal.so}}.  Take a look at how openssl does this:

{code}
      if (OpensslCipher.getLoadingFailureReason() != null) {
        openSslDetail = OpensslCipher.getLoadingFailureReason();
        openSslLoaded = false;
      } else {
        openSslDetail = OpensslCipher.getLibraryName();
        openSslLoaded = true;
      }
{code}

This is nice because {{hadoop checknative}} will display exactly *why* openssl 
can't be loaded (i.e. no build support, can't find library, library version 
incompatible, etc.) rather than just saying "not loaded" and leaving admins to 
guess why.

Also, you should display the full path to the isal-l library, not just a 
constant string.
Example output:

{code}
Native library checking:
hadoop:  true /h/lib/native/libhadoop.so.1.0.0
zlib:    true /lib64/libz.so.1
snappy:  true /usr/local/lib64/libsnappy.so.1
{code}

This may be important if I am getting a version that is different than what I 
thought (for example, perhaps I thought I was getting 
{{/opt/snappy/libsnappy.so.1}}, but I am actually getting 
{{/usr/local/lib64/libsnappy.so.1}})  Take a look at {{SnappyCompressor.c}} to 
see how to get the actual library path.

{code}
+                        if [ "${bundle.isal}" = "true" ] ; then
+                          cd "${isal.lib}"
+                          $$TAR *isa* | (cd $${TARGET_DIR}/; $$UNTAR)
+                        fi
{code}
Should be {{if \[ "x$\{bundle.isal\}" = x"true" \] ;}} to avoid the shell 
getting a syntax error when {{bundle.isal}} is not set.

There are a few cases where you have a space at the end of a line, which may 
trigger checkstyle warnings.

Thanks

> Introduce Intel ISA-L erasure coding library for the native support
> -------------------------------------------------------------------
>
>                 Key: HADOOP-11887
>                 URL: https://issues.apache.org/jira/browse/HADOOP-11887
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: io
>            Reporter: Kai Zheng
>            Assignee: Kai Zheng
>         Attachments: HADOOP-11887-v1.patch, HADOOP-11887-v2.patch, 
> HADOOP-11887-v3.patch, HADOOP-11887-v4.patch, HADOOP-11887-v5.patch
>
>
> This is to introduce Intel ISA-L erasure coding library for the native 
> support, via dynamic loading mechanism (dynamic module, like *.so in *nix and 
> *.dll on Windows).



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