[ https://issues.apache.org/jira/browse/HADOOP-19161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17864809#comment-17864809 ]
ASF GitHub Bot commented on HADOOP-19161: ----------------------------------------- mukund-thakur commented on code in PR #6789: URL: https://github.com/apache/hadoop/pull/6789#discussion_r1672860768 ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ConfigurationHelper.java: ########## @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.util; + +import java.util.EnumSet; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.stream.Collectors; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.VisibleForTesting; +import org.apache.hadoop.conf.Configuration; + +import static java.util.EnumSet.noneOf; +import static org.apache.hadoop.util.Preconditions.checkArgument; +import static org.apache.hadoop.util.StringUtils.getTrimmedStringCollection; + +/** + * Configuration Helper class to provide advanced configuration parsing. + * Private; external code MUST use {@link Configuration} instead + */ +@InterfaceAudience.Private +public final class ConfigurationHelper { + + /** + * Error string if there are multiple enum elements which only differ + * by case: {@value}. + */ + @VisibleForTesting + static final String ERROR_MULTIPLE_ELEMENTS_MATCHING_TO_LOWER_CASE_VALUE = + "has multiple elements matching to lower case value"; + + private ConfigurationHelper() { + } + + /** + * Given a comma separated list of enum values, + * trim the list, map to enum values in the message (case insensitive) + * and return the set. + * Special handling of "*" meaning: all values. + * @param key key for error messages. Review Comment: don't see much use of key param. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestConfigurationHelper.java: ########## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.util; + +import java.util.Set; + +import org.assertj.core.api.Assertions; +import org.assertj.core.api.IterableAssert; +import org.junit.Test; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.test.AbstractHadoopTestBase; + +import static org.apache.hadoop.test.LambdaTestUtils.intercept; +import static org.apache.hadoop.util.ConfigurationHelper.ERROR_MULTIPLE_ELEMENTS_MATCHING_TO_LOWER_CASE_VALUE; +import static org.apache.hadoop.util.ConfigurationHelper.mapEnumNamesToValues; +import static org.apache.hadoop.util.ConfigurationHelper.parseEnumSet; + +/** + * Test for {@link ConfigurationHelper}. + */ +public class TestConfigurationHelper extends AbstractHadoopTestBase { + + /** + * Simple Enums. + * "i" is included for case tests, as it is special in turkey. + */ + private enum SimpleEnum { a, b, c, i } + + + /** + * Special case: an enum with no values. + */ + private enum EmptyEnum { } + + /** + * Create assertion about the outcome of + * {@link ConfigurationHelper#parseEnumSet(String, String, Class, boolean)}. + * @param valueString value from Configuration + * @param enumClass class of enum + * @param ignoreUnknown should unknown values be ignored? + * @param <E> enum type + * @return an assertion on the outcome. + * @throws IllegalArgumentException if one of the entries was unknown and ignoreUnknown is false, + * or there are two entries in the enum which differ only by case. + */ + private static <E extends Enum<E>> IterableAssert<E> assertEnumParse( + final String valueString, + final Class<E> enumClass, + final boolean ignoreUnknown) { + final Set<E> enumSet = parseEnumSet("key", valueString, enumClass, ignoreUnknown); + final IterableAssert<E> assertion = Assertions.assertThat(enumSet); + return assertion.describedAs("parsed enum set '%s'", valueString); + } + + + /** + * Create a configuration with the key {@code key} set to a {@code value}. + * @param value value for the key + * @return a configuration with only key set. + */ + private Configuration confWithKey(String value) { + final Configuration conf = new Configuration(false); + conf.set("key", value); + return conf; + } + + @Test + public void testEnumParseAll() throws Throwable { + assertEnumParse("*", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i); + } + + @Test + public void testEnumParse() throws Throwable { + assertEnumParse("a, b,c", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c); + } + + @Test + public void testEnumCaseIndependence() throws Throwable { + assertEnumParse("A, B, C, I", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i); + } + + @Test + public void testEmptyArguments() throws Throwable { + assertEnumParse(" ", SimpleEnum.class, false) + .isEmpty(); + } + + @Test + public void testUnknownEnumNotIgnored() throws Throwable { + intercept(IllegalArgumentException.class, "unrecognized", () -> + parseEnumSet("key", "c, unrecognized", SimpleEnum.class, false)); + } + + @Test + public void testUnknownEnumNotIgnoredThroughConf() throws Throwable { + intercept(IllegalArgumentException.class, "unrecognized", () -> + confWithKey("c, unrecognized") + .getEnumSet("key", SimpleEnum.class, false)); + } + + @Test + public void testUnknownEnumIgnored() throws Throwable { + assertEnumParse("c, d", SimpleEnum.class, true) + .containsExactly(SimpleEnum.c); + } + + @Test + public void testStarEnum() throws Throwable { Review Comment: this test is already present above testEnumParseAll ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestConfigurationHelper.java: ########## @@ -0,0 +1,173 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.util; + +import java.util.Set; + +import org.assertj.core.api.Assertions; +import org.assertj.core.api.IterableAssert; +import org.junit.Test; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.test.AbstractHadoopTestBase; + +import static org.apache.hadoop.test.LambdaTestUtils.intercept; +import static org.apache.hadoop.util.ConfigurationHelper.ERROR_MULTIPLE_ELEMENTS_MATCHING_TO_LOWER_CASE_VALUE; +import static org.apache.hadoop.util.ConfigurationHelper.mapEnumNamesToValues; +import static org.apache.hadoop.util.ConfigurationHelper.parseEnumSet; + +/** + * Test for {@link ConfigurationHelper}. + */ +public class TestConfigurationHelper extends AbstractHadoopTestBase { + + /** + * Simple Enums. + * "i" is included for case tests, as it is special in turkey. + */ + private enum SimpleEnum { a, b, c, i } + + + /** + * Special case: an enum with no values. + */ + private enum EmptyEnum { } + + /** + * Create assertion about the outcome of + * {@link ConfigurationHelper#parseEnumSet(String, String, Class, boolean)}. + * @param valueString value from Configuration + * @param enumClass class of enum + * @param ignoreUnknown should unknown values be ignored? + * @param <E> enum type + * @return an assertion on the outcome. + * @throws IllegalArgumentException if one of the entries was unknown and ignoreUnknown is false, + * or there are two entries in the enum which differ only by case. + */ + private static <E extends Enum<E>> IterableAssert<E> assertEnumParse( + final String valueString, + final Class<E> enumClass, + final boolean ignoreUnknown) { + final Set<E> enumSet = parseEnumSet("key", valueString, enumClass, ignoreUnknown); + final IterableAssert<E> assertion = Assertions.assertThat(enumSet); + return assertion.describedAs("parsed enum set '%s'", valueString); + } + + + /** + * Create a configuration with the key {@code key} set to a {@code value}. + * @param value value for the key + * @return a configuration with only key set. + */ + private Configuration confWithKey(String value) { + final Configuration conf = new Configuration(false); + conf.set("key", value); + return conf; + } + + @Test + public void testEnumParseAll() throws Throwable { + assertEnumParse("*", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i); + } + + @Test + public void testEnumParse() throws Throwable { + assertEnumParse("a, b,c", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c); + } + + @Test + public void testEnumCaseIndependence() throws Throwable { + assertEnumParse("A, B, C, I", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i); + } + + @Test + public void testEmptyArguments() throws Throwable { + assertEnumParse(" ", SimpleEnum.class, false) + .isEmpty(); + } + + @Test + public void testUnknownEnumNotIgnored() throws Throwable { + intercept(IllegalArgumentException.class, "unrecognized", () -> + parseEnumSet("key", "c, unrecognized", SimpleEnum.class, false)); + } + + @Test + public void testUnknownEnumNotIgnoredThroughConf() throws Throwable { + intercept(IllegalArgumentException.class, "unrecognized", () -> + confWithKey("c, unrecognized") + .getEnumSet("key", SimpleEnum.class, false)); + } + + @Test + public void testUnknownEnumIgnored() throws Throwable { + assertEnumParse("c, d", SimpleEnum.class, true) + .containsExactly(SimpleEnum.c); + } + + @Test + public void testStarEnum() throws Throwable { + assertEnumParse("*", SimpleEnum.class, false) + .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i); + } + + @Test + public void testUnknownStarEnum() throws Throwable { Review Comment: add a test with repeated values "a, b, a". should pass ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ConfigurationHelper.java: ########## @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.util; + +import java.util.EnumSet; +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.stream.Collectors; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.VisibleForTesting; +import org.apache.hadoop.conf.Configuration; + +import static java.util.EnumSet.noneOf; +import static org.apache.hadoop.util.Preconditions.checkArgument; +import static org.apache.hadoop.util.StringUtils.getTrimmedStringCollection; + +/** + * Configuration Helper class to provide advanced configuration parsing. + * Private; external code MUST use {@link Configuration} instead + */ +@InterfaceAudience.Private +public final class ConfigurationHelper { + + /** + * Error string if there are multiple enum elements which only differ + * by case: {@value}. + */ + @VisibleForTesting + static final String ERROR_MULTIPLE_ELEMENTS_MATCHING_TO_LOWER_CASE_VALUE = + "has multiple elements matching to lower case value"; + + private ConfigurationHelper() { + } + + /** + * Given a comma separated list of enum values, + * trim the list, map to enum values in the message (case insensitive) + * and return the set. + * Special handling of "*" meaning: all values. + * @param key key for error messages. Review Comment: okay I get it now. maybe mention - configuration key which was used to configure the flags. I got confused initially because of the UT, ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java: ########## @@ -0,0 +1,278 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.impl; + +import java.util.Arrays; +import java.util.EnumSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Collectors; +import javax.annotation.Nullable; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.StreamCapabilities; +import org.apache.hadoop.util.ConfigurationHelper; +import org.apache.hadoop.util.Preconditions; + +import static org.apache.hadoop.util.ConfigurationHelper.mapEnumNamesToValues; + +/** + * A set of flags, constructed from a configuration option or from a string, + * with the semantics of + * {@link ConfigurationHelper#parseEnumSet(String, String, Class, boolean)} + * and implementing {@link StreamCapabilities}. + * <p> + * Thread safety: there is no synchronization on a mutable {@code FlagSet}. + * Once declared immutable, flags cannot be changed, so they + * becomes implicitly thread-safe. + */ +public final class FlagSet<E extends Enum<E>> implements StreamCapabilities { + + /** + * Set of flags. + */ + private final Set<E> flags; + + /** + * Is the set immutable? + */ + private final AtomicBoolean immutable = new AtomicBoolean(false); Review Comment: Isn't this class supposed to be singleton initialized only once during the file system creation while the configuration is being loaded? And read only flags such that callers can know if a falg is enabled or not? wondering what is the use case of updating the flags? > S3A: option "fs.s3a.performance.flags" to take list of performance flags > ------------------------------------------------------------------------ > > Key: HADOOP-19161 > URL: https://issues.apache.org/jira/browse/HADOOP-19161 > Project: Hadoop Common > Issue Type: Improvement > Components: fs/s3 > Affects Versions: 3.4.1 > Reporter: Steve Loughran > Assignee: Steve Loughran > Priority: Major > Labels: pull-request-available > > HADOOP-19072 shows we want to add more optimisations than that of > HADOOP-18930. > * Extending the new optimisations to the existing option is brittle > * Adding explicit options for each feature gets complext fast. > Proposed > * A new class S3APerformanceFlags keeps all the flags > * it build this from a string[] of values, which can be extracted from > getConf(), > * and it can also support a "*" option to mean "everything" > * this class can also be handed off to hasPathCapability() and do the right > thing. > Proposed optimisations > * create file (we will hook up HADOOP-18930) > * mkdir (HADOOP-19072) > * delete (probe for parent path) > * rename (probe for source path) > We could think of more, with different names, later. > The goal is make it possible to strip out every HTTP request we do for > safety/posix compliance, so applications have the option of turning off what > they don't need. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org