[
https://issues.apache.org/jira/browse/HDFS-16965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17707099#comment-17707099
]
ASF GitHub Bot commented on HDFS-16965:
---------------------------------------
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.
> Add switch to decide whether to enable native codec.
> ----------------------------------------------------
>
> Key: HDFS-16965
> URL: https://issues.apache.org/jira/browse/HDFS-16965
> Project: Hadoop HDFS
> Issue Type: New Feature
> Components: erasure-coding
> Affects Versions: 3.3.4
> Reporter: WangYuanben
> Priority: Minor
> Labels: pull-request-available
>
> Sometimes we need to create codec without ISA-L, while priority is given to
> native codec by default. So it is necessary to add switch to decide whether
> to enable native codec.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
