[
https://issues.apache.org/jira/browse/HADOOP-19161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17867110#comment-17867110
]
ASF GitHub Bot commented on HADOOP-19161:
-----------------------------------------
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
> 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
