hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-11887) Introduce Intel ISA-L erasure coding library for the native support
Date Thu, 30 Jul 2015 21:04:05 GMT

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

Mime
View raw message