[GitHub] [hadoop] steveloughran commented on a change in pull request #2350: HADOOP-17292. Using lz4-java in Lz4Codec

[GitBox <gi...@apache.org>]

steveloughran commented on a change in pull request #2350:
URL: https://github.com/apache/hadoop/pull/2350#discussion_r502553101



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/lz4/Lz4Compressor.java
##########
@@ -50,20 +51,7 @@
 
   private final boolean useLz4HC;
 
-  static {
-    if (NativeCodeLoader.isNativeCodeLoaded()) {
-      // Initialize the native library
-      try {
-        initIDs();
-      } catch (Throwable t) {
-        // Ignore failure to load/initialize lz4
-        LOG.warn(t.toString());
-      }
-    } else {
-      LOG.error("Cannot load " + Lz4Compressor.class.getName() +
-          " without native hadoop library!");
-    }
-  }
+  private LZ4Compressor lz4Compressor;

Review comment:
       final?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/lz4/Lz4Compressor.java
##########
@@ -22,9 +22,10 @@
 import java.nio.Buffer;
 import java.nio.ByteBuffer;
 
+import net.jpountz.lz4.LZ4Factory;
+import net.jpountz.lz4.LZ4Compressor;

Review comment:
       put into the same import block as org.sjf4j

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.java
##########
@@ -22,8 +22,9 @@
 import java.nio.Buffer;
 import java.nio.ByteBuffer;
 
+import net.jpountz.lz4.LZ4Factory;
+import net.jpountz.lz4.LZ4SafeDecompressor;

Review comment:
       again, not in same import block as org.apache

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.java
##########
@@ -67,6 +55,15 @@
   public Lz4Decompressor(int directBufferSize) {
     this.directBufferSize = directBufferSize;
 
+    try {
+      LZ4Factory lz4Factory = LZ4Factory.fastestInstance();
+      lz4Decompressor = lz4Factory.safeDecompressor();
+    } catch (Throwable t) {
+      throw new RuntimeException("lz4-java library is not available: " +
+              "Lz4Decompressor has not been loaded. You need to add " +
+              "lz4-java.jar to your CLASSPATH", t);

Review comment:
       add +t to the end of the string, so the specific error text isn't lost

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.java
##########
@@ -67,6 +55,15 @@
   public Lz4Decompressor(int directBufferSize) {
     this.directBufferSize = directBufferSize;
 
+    try {
+      LZ4Factory lz4Factory = LZ4Factory.fastestInstance();
+      lz4Decompressor = lz4Factory.safeDecompressor();
+    } catch (Throwable t) {

Review comment:
       Shame the constructor can't throw exceptions direct.
   
   Catch only the exception's known to be raised, and don't convert RTEs or, especially, Errors. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org