virajjasani commented on code in PR #6789:
URL: https://github.com/apache/hadoop/pull/6789#discussion_r1683479251
##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/performance.md:
##########
@@ -276,7 +283,45 @@ Fix: Use one of the dedicated [S3A
Committers](committers.md).
## <a name="tuning"></a> Options to Tune
-### <a name="pooling"></a> Thread and connection pool settings.
+### <a name="flags"></a> Performance Flags: `fs.s3a.performance.flag`
+
+This option takes a comma separated list of performance flags.
+View it as the equivalent of the `-O` compiler optimization list C/C++
compilers offer.
+That is a complicated list of options which deliver speed if the person
setting them
+understands the risks.
+
+* The list of flags MAY change across releases
+* The semantics of specific flags SHOULD NOT change across releases.
+* If an option is to be tuned which may relax semantics, a new option MUST be
defined.
+* Unknown flags are ignored; this is to avoid compatibility.
+* The option `*` means "turn everything on". This is implicitly unstable
across releases.
+
+| *Option* | *Meaning* | Since |
+|----------|--------------------|:------|
+| `create` | Create Performance | 3.4.1 |
Review Comment:
nit: shall we mention `3.4.1 / 3.5.0`?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -2209,7 +2237,8 @@ public FSDataOutputStreamBuilder createFile(final Path
path) {
builder
.create()
.overwrite(true)
- .must(FS_S3A_CREATE_PERFORMANCE, performanceCreation);
+ .must(FS_S3A_CREATE_PERFORMANCE,
+ getPerformanceFlags().enabled(PerformanceFlagEnum.Create));
Review Comment:
just a thought not related to this patch, but shall we make
`FSDataOutputStreamBuilder` more specific for s3afs
e.g.
`public FSDataOutputStreamBuilder<FSDataOutputStream, CreateFileBuilder>
createFile(final Path path)`
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestFlagSet.java:
##########
@@ -0,0 +1,431 @@
+/*
+ * 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.EnumSet;
+
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static java.util.EnumSet.allOf;
+import static java.util.EnumSet.noneOf;
+import static org.apache.hadoop.fs.impl.FlagSet.buildFlagSet;
+import static org.apache.hadoop.fs.impl.FlagSet.createFlagSet;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+
+/**
+ * Unit tests for {@link FlagSet} class.
+ */
+public final class TestFlagSet extends AbstractHadoopTestBase {
+
+ private static final String KEY = "key";
+
+ public static final String CAPABILITY_B = KEY + ".b";
+
+ public static final String CAPABILITY_C = KEY + ".c";
+
+ public static final String CAPABILITY_A = KEY + ".a";
+
+ private static final String KEYDOT = KEY + ".";
+
+ /**
+ * Flagset used in tests and assertions.
+ */
+ private FlagSet<SimpleEnum> flagSet =
+ createFlagSet(SimpleEnum.class, KEYDOT, noneOf(SimpleEnum.class));
+
+ /**
+ * Simple Enums for the tests.
+ */
+ private enum SimpleEnum { a, b, c }
+
+ /**
+ * Enum with a single value.
+ */
+ private enum OtherEnum { a }
+
+ /**
+ * Test that an entry can be enabled and disabled.
+ */
+ @Test
+ public void testEntryEnableDisable() {
+ Assertions.assertThat(flagSet.flags()).isEmpty();
+ assertDisabled(SimpleEnum.a);
+ flagSet.enable(SimpleEnum.a);
+ assertEnabled(SimpleEnum.a);
+ flagSet.disable(SimpleEnum.a);
+ assertDisabled(SimpleEnum.a);
+ }
+
+ /**
+ * Test the setter.
+ */
+ @Test
+ public void testSetMethod() {
+ Assertions.assertThat(flagSet.flags()).isEmpty();
+ flagSet.set(SimpleEnum.a, true);
+ assertEnabled(SimpleEnum.a);
+ flagSet.set(SimpleEnum.a, false);
+ assertDisabled(SimpleEnum.a);
+ }
+
+ /**
+ * Test mutability by making immutable and
+ * expecting setters to fail.
+ */
+ @Test
+ public void testMutability() throws Throwable {
+ flagSet.set(SimpleEnum.a, true);
+ flagSet.makeImmutable();
+ intercept(IllegalStateException.class, () ->
+ flagSet.disable(SimpleEnum.a));
+ assertEnabled(SimpleEnum.a);
+ intercept(IllegalStateException.class, () ->
+ flagSet.set(SimpleEnum.a, false));
+ assertEnabled(SimpleEnum.a);
+ // now look at the setters
+ intercept(IllegalStateException.class, () ->
+ flagSet.enable(SimpleEnum.b));
+ assertDisabled(SimpleEnum.b);
+ intercept(IllegalStateException.class, () ->
+ flagSet.set(SimpleEnum.b, true));
+ assertDisabled(SimpleEnum.b);
+ }
+
+ /**
+ * Test stringification.
+ */
+ @Test
+ public void testToString() throws Throwable {
+ // empty
+ assertStringValue("{}");
+ assertConfigurationStringMatches("");
+
+ // single value
+ flagSet.enable(SimpleEnum.a);
+ assertStringValue("{a}");
+ assertConfigurationStringMatches("a");
+
+ // add a second value.
+ flagSet.enable(SimpleEnum.b);
+ assertStringValue("{a, b}");
+ }
+
+ /**
+ * Assert that {@link FlagSet#toString()} matches the expected
+ * value.
+ * @param expected expected value
+ */
+ private void assertStringValue(final String expected) {
+ Assertions.assertThat(flagSet.toString())
+ .isEqualTo(expected);
+ }
+
+ /**
+ * Assert the configuration string form matches that expected.
+ */
+ public void assertConfigurationStringMatches(final String expected) {
+ Assertions.assertThat(flagSet.toConfigurationString())
+ .describedAs("Configuration string of %s", flagSet)
+ .isEqualTo(expected);
+ }
+
+ /**
+ * Test parsing from a configuration file.
+ * Multiple entries must be parsed, whitespace trimmed.
+ */
+ @Test
+ public void testConfEntry() {
+ flagSet = flagSetFromConfig("a\t,\nc ", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a, SimpleEnum.c);
+ assertHasCapability(CAPABILITY_A);
+ assertHasCapability(CAPABILITY_C);
+ assertLacksCapability(CAPABILITY_B);
+ assertPathCapabilitiesMatch(flagSet, CAPABILITY_A, CAPABILITY_C);
+ }
+
+ /**
+ * Create a flagset from a configuration string.
+ * @param string configuration string.
+ * @param ignoreUnknown should unknown values be ignored?
+ * @return a flagset
+ */
+ private static FlagSet<SimpleEnum> flagSetFromConfig(final String string,
+ final boolean ignoreUnknown) {
+ final Configuration conf = mkConf(string);
+ return buildFlagSet(SimpleEnum.class, conf, KEY, ignoreUnknown);
+ }
+
+ /**
+ * Test parsing from a configuration file,
+ * where an entry is unknown; the builder is set to ignoreUnknown.
+ */
+ @Test
+ public void testConfEntryWithUnknownIgnored() {
+ flagSet = flagSetFromConfig("a, unknown", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a);
+ assertHasCapability(CAPABILITY_A);
+ assertLacksCapability(CAPABILITY_B);
+ assertLacksCapability(CAPABILITY_C);
+ }
+
+ /**
+ * Test parsing from a configuration file where
+ * the same entry is duplicated.
+ */
+ @Test
+ public void testDuplicateConfEntry() {
+ flagSet = flagSetFromConfig("a,\ta,\na\"", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a);
+ assertHasCapability(CAPABILITY_A);
+ }
+
+ /**
+ * Handle an unknown configuration value.
+ */
+ @Test
+ public void testConfUnknownFailure() throws Throwable {
+ intercept(IllegalArgumentException.class, () ->
+ flagSetFromConfig("a, unknown", false));
+ }
+
+ /**
+ * Create a configuration with {@link #KEY} set to the given value.
+ * @param value value to set
+ * @return the configuration.
+ */
+ private static Configuration mkConf(final String value) {
+ final Configuration conf = new Configuration(false);
+ conf.set(KEY, value);
+ return conf;
+ }
+
+ /**
+ * Assert that the flagset has a capability.
+ * @param capability capability to probe for
+ */
+ private void assertHasCapability(final String capability) {
+ Assertions.assertThat(flagSet.hasCapability(capability))
+ .describedAs("Capabiilty of %s on %s", capability, flagSet)
Review Comment:
nit: `Capability`
##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/performance.md:
##########
@@ -276,7 +283,45 @@ Fix: Use one of the dedicated [S3A
Committers](committers.md).
## <a name="tuning"></a> Options to Tune
-### <a name="pooling"></a> Thread and connection pool settings.
+### <a name="flags"></a> Performance Flags: `fs.s3a.performance.flag`
+
+This option takes a comma separated list of performance flags.
+View it as the equivalent of the `-O` compiler optimization list C/C++
compilers offer.
+That is a complicated list of options which deliver speed if the person
setting them
+understands the risks.
Review Comment:
nit: just in case if it looks better? `if the person or client application
setting them`
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestFlagSet.java:
##########
@@ -0,0 +1,431 @@
+/*
+ * 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.EnumSet;
+
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static java.util.EnumSet.allOf;
+import static java.util.EnumSet.noneOf;
+import static org.apache.hadoop.fs.impl.FlagSet.buildFlagSet;
+import static org.apache.hadoop.fs.impl.FlagSet.createFlagSet;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+
+/**
+ * Unit tests for {@link FlagSet} class.
+ */
+public final class TestFlagSet extends AbstractHadoopTestBase {
+
+ private static final String KEY = "key";
+
+ public static final String CAPABILITY_B = KEY + ".b";
+
+ public static final String CAPABILITY_C = KEY + ".c";
+
+ public static final String CAPABILITY_A = KEY + ".a";
+
+ private static final String KEYDOT = KEY + ".";
+
+ /**
+ * Flagset used in tests and assertions.
+ */
+ private FlagSet<SimpleEnum> flagSet =
+ createFlagSet(SimpleEnum.class, KEYDOT, noneOf(SimpleEnum.class));
+
+ /**
+ * Simple Enums for the tests.
+ */
+ private enum SimpleEnum { a, b, c }
+
+ /**
+ * Enum with a single value.
+ */
+ private enum OtherEnum { a }
+
+ /**
+ * Test that an entry can be enabled and disabled.
+ */
+ @Test
+ public void testEntryEnableDisable() {
+ Assertions.assertThat(flagSet.flags()).isEmpty();
+ assertDisabled(SimpleEnum.a);
+ flagSet.enable(SimpleEnum.a);
+ assertEnabled(SimpleEnum.a);
+ flagSet.disable(SimpleEnum.a);
+ assertDisabled(SimpleEnum.a);
+ }
+
+ /**
+ * Test the setter.
+ */
+ @Test
+ public void testSetMethod() {
+ Assertions.assertThat(flagSet.flags()).isEmpty();
+ flagSet.set(SimpleEnum.a, true);
+ assertEnabled(SimpleEnum.a);
+ flagSet.set(SimpleEnum.a, false);
+ assertDisabled(SimpleEnum.a);
+ }
+
+ /**
+ * Test mutability by making immutable and
+ * expecting setters to fail.
+ */
+ @Test
+ public void testMutability() throws Throwable {
+ flagSet.set(SimpleEnum.a, true);
+ flagSet.makeImmutable();
+ intercept(IllegalStateException.class, () ->
+ flagSet.disable(SimpleEnum.a));
+ assertEnabled(SimpleEnum.a);
+ intercept(IllegalStateException.class, () ->
+ flagSet.set(SimpleEnum.a, false));
+ assertEnabled(SimpleEnum.a);
+ // now look at the setters
+ intercept(IllegalStateException.class, () ->
+ flagSet.enable(SimpleEnum.b));
+ assertDisabled(SimpleEnum.b);
+ intercept(IllegalStateException.class, () ->
+ flagSet.set(SimpleEnum.b, true));
+ assertDisabled(SimpleEnum.b);
+ }
+
+ /**
+ * Test stringification.
+ */
+ @Test
+ public void testToString() throws Throwable {
+ // empty
+ assertStringValue("{}");
+ assertConfigurationStringMatches("");
+
+ // single value
+ flagSet.enable(SimpleEnum.a);
+ assertStringValue("{a}");
+ assertConfigurationStringMatches("a");
+
+ // add a second value.
+ flagSet.enable(SimpleEnum.b);
+ assertStringValue("{a, b}");
+ }
+
+ /**
+ * Assert that {@link FlagSet#toString()} matches the expected
+ * value.
+ * @param expected expected value
+ */
+ private void assertStringValue(final String expected) {
+ Assertions.assertThat(flagSet.toString())
+ .isEqualTo(expected);
+ }
+
+ /**
+ * Assert the configuration string form matches that expected.
+ */
+ public void assertConfigurationStringMatches(final String expected) {
+ Assertions.assertThat(flagSet.toConfigurationString())
+ .describedAs("Configuration string of %s", flagSet)
+ .isEqualTo(expected);
+ }
+
+ /**
+ * Test parsing from a configuration file.
+ * Multiple entries must be parsed, whitespace trimmed.
+ */
+ @Test
+ public void testConfEntry() {
+ flagSet = flagSetFromConfig("a\t,\nc ", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a, SimpleEnum.c);
+ assertHasCapability(CAPABILITY_A);
+ assertHasCapability(CAPABILITY_C);
+ assertLacksCapability(CAPABILITY_B);
+ assertPathCapabilitiesMatch(flagSet, CAPABILITY_A, CAPABILITY_C);
+ }
+
+ /**
+ * Create a flagset from a configuration string.
+ * @param string configuration string.
+ * @param ignoreUnknown should unknown values be ignored?
+ * @return a flagset
+ */
+ private static FlagSet<SimpleEnum> flagSetFromConfig(final String string,
+ final boolean ignoreUnknown) {
+ final Configuration conf = mkConf(string);
+ return buildFlagSet(SimpleEnum.class, conf, KEY, ignoreUnknown);
+ }
+
+ /**
+ * Test parsing from a configuration file,
+ * where an entry is unknown; the builder is set to ignoreUnknown.
+ */
+ @Test
+ public void testConfEntryWithUnknownIgnored() {
+ flagSet = flagSetFromConfig("a, unknown", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a);
+ assertHasCapability(CAPABILITY_A);
+ assertLacksCapability(CAPABILITY_B);
+ assertLacksCapability(CAPABILITY_C);
+ }
+
+ /**
+ * Test parsing from a configuration file where
+ * the same entry is duplicated.
+ */
+ @Test
+ public void testDuplicateConfEntry() {
+ flagSet = flagSetFromConfig("a,\ta,\na\"", true);
+ assertFlagSetMatches(flagSet, SimpleEnum.a);
+ assertHasCapability(CAPABILITY_A);
+ }
+
+ /**
+ * Handle an unknown configuration value.
+ */
+ @Test
+ public void testConfUnknownFailure() throws Throwable {
+ intercept(IllegalArgumentException.class, () ->
+ flagSetFromConfig("a, unknown", false));
+ }
+
+ /**
+ * Create a configuration with {@link #KEY} set to the given value.
+ * @param value value to set
+ * @return the configuration.
+ */
+ private static Configuration mkConf(final String value) {
+ final Configuration conf = new Configuration(false);
+ conf.set(KEY, value);
+ return conf;
+ }
+
+ /**
+ * Assert that the flagset has a capability.
+ * @param capability capability to probe for
+ */
+ private void assertHasCapability(final String capability) {
+ Assertions.assertThat(flagSet.hasCapability(capability))
+ .describedAs("Capabiilty of %s on %s", capability, flagSet)
+ .isTrue();
+ }
+
+ /**
+ * Assert that the flagset lacks a capability.
+ * @param capability capability to probe for
+ */
+ private void assertLacksCapability(final String capability) {
+ Assertions.assertThat(flagSet.hasCapability(capability))
+ .describedAs("Capabiilty of %s on %s", capability, flagSet)
Review Comment:
same here
--
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
