[GitHub] [accumulo] ctubbsii commented on a change in pull request #2417: Use Hadoop compression codec override (#2416)

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

ctubbsii commented on a change in pull request #2417:
URL: https://github.com/apache/accumulo/pull/2417#discussion_r786960143



##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -761,16 +761,20 @@ CompressionCodec initCodec(final AtomicBoolean checked, final int bufferSize,
      */
     CompressionCodec createNewCodec(final String codecClazzProp, final String defaultClazz,
         final int bufferSize, final String bufferSizeConfigOpt) {
-      String extClazz =
-          (conf.get(codecClazzProp) == null ? System.getProperty(codecClazzProp) : null);
+      String extClazz = conf.get(codecClazzProp);
+      if (extClazz == null) {
+        extClazz = System.getProperty(codecClazzProp);
+      }
       String clazz = (extClazz != null) ? extClazz : defaultClazz;

Review comment:
       I don't think this behaves the way it should. This would get the class from the config file, and only use the system property if it's not set in the config file. I think it should work the other way around. Setting the system property on the command-line, for example, should be able to override what is set in the config file.
   
   ```suggestion
         String clazz = System.getProperty(codecClazzProp, conf.get(codecClazzProp, defaultClazz));
   ```
   
   Also, this syntax is much more readable: try system props, fall back to config, fall back to default.

##########
File path: core/src/test/java/org/apache/accumulo/core/file/rfile/bcfile/CompressionTest.java
##########
@@ -282,4 +299,27 @@ public void testThereCanBeOnlyOne() throws InterruptedException, ExecutionExcept
     }
   }
 
+  @Test
+  public void testHadoopCodecOverride() {
+    Algorithm.conf.set(Compression.Algorithm.CONF_LZ4_CLASS, DummyCodec.class.getName());
+    CompressionCodec dummyCodec = Compression.Algorithm.LZ4.createNewCodec(4096);
+    assertEquals("Hadoop override DummyCodec not loaded", DummyCodec.class, dummyCodec.getClass());

Review comment:
       These tests seem a bit heavyweight for what they are effectively testing. They are effectively testing that we can read the class name from the config files and/or system property. We don't actually need to create the codec to test that. These tests can probably be made more lightweight. However, given the triviality of my proposed one-liner above to get the class name, I'm not sure there's much to test here at all.

##########
File path: core/src/test/java/org/apache/accumulo/core/file/rfile/bcfile/CompressionTest.java
##########
@@ -119,7 +126,17 @@ public void testSupport() {
     } catch (ClassNotFoundException e) {
       // that is okay
     }
+  }
 
+  @After
+  public void restoreSysProps() {
+    // Restore system property for next test
+    Algorithm.conf.clear();
+    if (extLz4 != null) {
+      System.setProperty(Compression.Algorithm.CONF_LZ4_CLASS, extLz4);
+    } else {
+      System.clearProperty(Compression.Algorithm.CONF_LZ4_CLASS);
+    }

Review comment:
       We run our unit tests in parallel and reuse JVM forks to do so. Messing with system properties like this could make these tests not thread safe. But, I'm not sure.

##########
File path: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java
##########
@@ -761,16 +761,20 @@ CompressionCodec initCodec(final AtomicBoolean checked, final int bufferSize,
      */
     CompressionCodec createNewCodec(final String codecClazzProp, final String defaultClazz,
         final int bufferSize, final String bufferSizeConfigOpt) {
-      String extClazz =
-          (conf.get(codecClazzProp) == null ? System.getProperty(codecClazzProp) : null);
+      String extClazz = conf.get(codecClazzProp);
+      if (extClazz == null) {
+        extClazz = System.getProperty(codecClazzProp);
+      }
       String clazz = (extClazz != null) ? extClazz : defaultClazz;
       try {
         log.info("Trying to load codec class {} for {}", clazz, codecClazzProp);
         Configuration config = new Configuration(conf);
         updateBuffer(config, bufferSizeConfigOpt, bufferSize);
         return (CompressionCodec) ReflectionUtils.newInstance(Class.forName(clazz), config);
       } catch (ClassNotFoundException e) {
-        // This is okay.
+        // This is not okay.
+        log.warn("Unable to load codec class {} for {}, reason: {}", clazz, codecClazzProp,
+            e.getMessage());

Review comment:
       ```suggestion
           log.warn("Unable to load codec class {} for {}", clazz, codecClazzProp, e);
   ```




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

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