cnauroth commented on code in PR #5520:
URL: https://github.com/apache/hadoop/pull/5520#discussion_r1153980763
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/erasurecode/CodecUtil.java:
##########
@@ -170,8 +174,14 @@ private static String[] getRawCoderNames(
private static RawErasureEncoder createRawEncoderWithFallback(
Configuration conf, String codecName, ErasureCoderOptions coderOptions) {
+ boolean ISALEnabled =
conf.getBoolean(IO_ERASURECODE_CODEC_NATIVE_ENABLED_KEY,
+ IO_ERASURECODE_CODEC_NATIVE_ENABLED_DEFAULT);
String[] rawCoderNames = getRawCoderNames(conf, codecName);
for (String rawCoderName : rawCoderNames) {
+ if (!ISALEnabled && rawCoderName.split("_")[1].equals("native")) {
Review Comment:
Ideally, this would be made more resilient to unexpected input (e.g. a
configured coder name without an "_", resulting in an
`ArrayIndexOutOfBoundsException`.).
##########
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml:
##########
@@ -920,6 +920,14 @@
</description>
</property>
+<property>
+ <name>io.erasurecode.codec.native.enabled</name>
+ <value>true</value>
+ <description>
+ Used to decide whether to enable native codec.
Review Comment:
This needs more explanation. In what cases is it a good idea to disable
ISA-L support?
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/erasurecode/TestCodecRawCoderMapping.java:
##########
@@ -30,17 +30,22 @@
import org.apache.hadoop.io.erasurecode.rawcoder.RawErasureEncoder;
import org.apache.hadoop.io.erasurecode.rawcoder.XORRawDecoder;
import org.apache.hadoop.io.erasurecode.rawcoder.XORRawEncoder;
+import org.apache.hadoop.io.erasurecode.rawcoder.NativeXORRawEncoder;
+import org.apache.hadoop.io.erasurecode.rawcoder.NativeXORRawDecoder;
import org.apache.hadoop.io.erasurecode.rawcoder.XORRawErasureCoderFactory;
import org.apache.hadoop.test.GenericTestUtils;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test the codec to raw coder mapping.
*/
public class TestCodecRawCoderMapping {
-
+ public static final Logger LOG =
Review Comment:
Nitpick: Switch to `private`.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/erasurecode/TestCodecRawCoderMapping.java:
##########
@@ -150,4 +155,44 @@ public void testIgnoreInvalidCodec() {
conf, ErasureCodeConstants.XOR_CODEC_NAME, coderOptions);
Assert.assertTrue(decoder instanceof XORRawDecoder);
}
+
+ @Test
+ public void testCodecNativeEnabled() {
+ if (!ErasureCodeNative.isNativeCodeLoaded()) {
+ LOG.warn("ISA-L support is not available in your platform.");
+ return;
+ }
+ ErasureCoderOptions coderOptions = new ErasureCoderOptions(
+ numDataUnit, numParityUnit);
+
+ conf.setBoolean(CodecUtil.IO_ERASURECODE_CODEC_NATIVE_ENABLED_KEY,
+ CodecUtil.IO_ERASURECODE_CODEC_NATIVE_ENABLED_DEFAULT);
+ RawErasureEncoder rsEncoder = CodecUtil.createRawEncoder(
+ conf, ErasureCodeConstants.RS_CODEC_NAME, coderOptions);
+ RawErasureDecoder rsDecoder = CodecUtil.createRawDecoder(
+ conf, ErasureCodeConstants.RS_CODEC_NAME, coderOptions);
+ RawErasureEncoder xorEncoder = CodecUtil.createRawEncoder(
+ conf, ErasureCodeConstants.XOR_CODEC_NAME, coderOptions);
+ RawErasureDecoder xorDecoder = CodecUtil.createRawDecoder(
+ conf, ErasureCodeConstants.XOR_CODEC_NAME, coderOptions);
+ Assert.assertTrue(rsEncoder instanceof NativeRSRawEncoder);
Review Comment:
Not entirely related to your change, but the whole file would probably be
more readable with static imports of the assert methods.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/erasurecode/TestCodecRawCoderMapping.java:
##########
@@ -150,4 +155,44 @@ public void testIgnoreInvalidCodec() {
conf, ErasureCodeConstants.XOR_CODEC_NAME, coderOptions);
Assert.assertTrue(decoder instanceof XORRawDecoder);
}
+
+ @Test
+ public void testCodecNativeEnabled() {
+ if (!ErasureCodeNative.isNativeCodeLoaded()) {
Review Comment:
This looks more like a use case for JUnit
[`Assume#assumeTrue`](https://junit.org/junit4/javadoc/4.13/org/junit/Assume.html#assumeTrue(boolean)).
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/erasurecode/TestCodecRawCoderMapping.java:
##########
@@ -150,4 +155,44 @@ public void testIgnoreInvalidCodec() {
conf, ErasureCodeConstants.XOR_CODEC_NAME, coderOptions);
Assert.assertTrue(decoder instanceof XORRawDecoder);
}
+
+ @Test
+ public void testCodecNativeEnabled() {
+ if (!ErasureCodeNative.isNativeCodeLoaded()) {
+ LOG.warn("ISA-L support is not available in your platform.");
+ return;
+ }
+ ErasureCoderOptions coderOptions = new ErasureCoderOptions(
+ numDataUnit, numParityUnit);
+
+ conf.setBoolean(CodecUtil.IO_ERASURECODE_CODEC_NATIVE_ENABLED_KEY,
Review Comment:
Is this necessary? It's the default, so it seems it wouldn't be necessary to
set explicitly.
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
