HADOOP-19161. S3A: option "fs.s3a.performance.flags" to take list of performance flags #6789
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
note the commented out bit where we considered adding options like "hive" or "spark". @HarshitGupta11 and I discussed this; for now lets go with a list of options and "*" |
virajjasani
reviewed
May 2, 2024
Comment on lines
151
to
154
| /* case "hive": | ||
| case "impala": | ||
| case "spark": | ||
| case "distcp": |
There was a problem hiding this comment.
Should we not let downstreamers decide what flags they want (after extensive testing)? And across different releases, they might need different flags to be turned on (in case of any regression)?
We can just recommend the flags (as already commented out here) but not set the flags for them. Thoughts?
There was a problem hiding this comment.
harshit and I were discussing this. i think it's best to have that option list, as app settings could be too brittle to changes
Comment on lines
76
to
78
| public boolean isDelete() { | ||
| return delete; | ||
| } |
There was a problem hiding this comment.
This one also we want to tackle as separate task (after HADOOP-19072), correct?
There was a problem hiding this comment.
yes. harshit did an experiment where he turned off all attempts at creating parent dirs after delete. fairly brittle, i think
|
I have a better design for this. changign this to draft. Proposed: we have a this makes it trivial to reuse/process. the implementation would be outside the actual Configuration class to make it easy for AbfsConfiguration to use too |
steveloughran
force-pushed
the
s3/HADOOP-19161-performance-flags
branch
from
feb384f to
28cd50a
Compare
|
steveloughran
force-pushed
the
s3/HADOOP-19161-performance-flags
branch
from
28cd50a to
f3571ac
Compare
steveloughran
force-pushed
the
s3/HADOOP-19161-performance-flags
branch
from
a564aa4 to
82974c4
Compare
|
@mukund-thakur @saxenapranav @HarshitGupta11 @ahmarsuhail @virajjasani FYI, this is ready to review. Once in, viraj's mkdirs option can go in, and then we can document this option in performance.md ABFS can use it for their options |
steveloughran
force-pushed
the
s3/HADOOP-19161-performance-flags
branch
2 times, most recently
from
206bfab to
af2dd8d
Compare
|
OK, I'm happy with this. @mukund-thakur @virajjasani thoughts? |
|
Sounds good, i would take a detailed look at ConfigurationHelper and FlagSet soon. |
|
realised we need one more thing: a way to enumerate all enabled flags. |
steveloughran
force-pushed
the
s3/HADOOP-19161-performance-flags
branch
from
d9cb6f9 to
64319c7
Compare
virajjasani
reviewed
Jul 2, 2024
Comment on lines
+91
to
+111
| intercept(IllegalArgumentException.class, "unrecognized", () -> | ||
| parseEnumSet("key", "c, unrecognized", SimpleEnum.class, false)); |
There was a problem hiding this comment.
How about intercepting the util method call instead of direct call to parseEnumSet:
intercept(IllegalArgumentException.class, "unrecognized", () -> assertEnumParse("c, unrecognized", SimpleEnum.class, false))?
There was a problem hiding this comment.
let me do that as a separate test
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java
Outdated
Show resolved
Hide resolved
| sb.append(", partSize=").append(partSize); | ||
| sb.append(", enableMultiObjectsDelete=").append(enableMultiObjectsDelete); | ||
| sb.append(", maxKeys=").append(maxKeys); | ||
| sb.append(", ").append(performanceFlags); |
There was a problem hiding this comment.
key performanceFlags= missing
Comment on lines
+104
to
+106
| public boolean enabled(final E flag) { | ||
| return flags.contains(flag); | ||
| } |
There was a problem hiding this comment.
Similar to how we check for mutability of flags while enabling/disabling a flag, i wonder if we should check for immutability while checking for enabled()? Probably not, but then how would client know when to set immutable? or maybe it's fine to not set immutability ever?
There was a problem hiding this comment.
If we need it immutable, probably it's good case for builder or constructor so maybe keeping the object mutable is also fine?
There was a problem hiding this comment.
i started off fully mutable, added immutabilty as an option to avoid all sync issues. you can manipulated it as much as you want and only once made immutable do you need to worry about thread safety etc. lets you buld by copying and updating, from a default value with changes etc
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestFlagSet.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Change-Id: Ie28b4ab01881cfe92fb622d4d27b7bdad8a690fd
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: Ifa8941acbcc84be30a997b6097947c302158f583
| * 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. |
There was a problem hiding this comment.
don't see much use of key param.
There was a problem hiding this 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,
| } | ||
|
|
||
| @Test | ||
| public void testStarEnum() throws Throwable { |
There was a problem hiding this comment.
this test is already present above testEnumParseAll
| } | ||
|
|
||
| @Test | ||
| public void testUnknownStarEnum() throws Throwable { |
There was a problem hiding this comment.
add a test with repeated values "a, b, a". should pass
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
@mukund-thakur thanks, altered the tests and tried to clarify the immutability. One final thought: should we have a default list of enum values to set if, when parsing the config string, the string isn't found? Currently if nothing is found we return the empty set. |
* Mukund's feedback * Remove unused "throws Throwable" on test cases as appropriate Save enumClass constructor parameter of FlagSet * add accessor * compare in equals * use in a copy() operation which creates a mutable deep copy of an object. * explicit checks for null enumclass, prefix in constructor Change-Id: I8080d772bfd301ab6e3ed802d54a060a63feea78
|
🎊 +1 overall
This message was automatically generated. |
I am good with an empty set. What is the default value you are thinking? create is already getting added based on the create perf flag explicitly. |
virajjasani
approved these changes
Jul 18, 2024
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Show resolved
Hide resolved
| */ | ||
| private void assertHasCapability(final String capability) { | ||
| Assertions.assertThat(flagSet.hasCapability(capability)) | ||
| .describedAs("Capabiilty of %s on %s", capability, flagSet) |
| */ | ||
| private void assertLacksCapability(final String capability) { | ||
| Assertions.assertThat(flagSet.hasCapability(capability)) | ||
| .describedAs("Capabiilty of %s on %s", capability, flagSet) |
|
|
||
| | *Option* | *Meaning* | Since | | ||
| |----------|--------------------|:------| | ||
| | `create` | Create Performance | 3.4.1 | |
There was a problem hiding this comment.
nit: shall we mention 3.4.1 / 3.5.0?
Comment on lines
+290
to
+291
| That is a complicated list of options which deliver speed if the person setting them | ||
| understands the risks. |
There was a problem hiding this comment.
nit: just in case if it looks better? if the person or client application setting them
There was a problem hiding this comment.
no, its an individual who takes the blame
Change-Id: Id49ceae8badaaa80e35dbe62c75a7d6d107449c1
|
🎊 +1 overall
This message was automatically generated. |
|
ok, everything is addressed, going to merge to the branches |
steveloughran
added a commit
to steveloughran/hadoop
that referenced
this pull request
Jul 29, 2024
…performance flags (apache#6789) 1. Configuration adds new method `getEnumSet()` to get a set of enum values from a configuration string. <E extends Enum<E>> EnumSet<E> getEnumSet(String key, Class<E> enumClass, boolean ignoreUnknown) Whitespace is ignored, case is ignored and the value "*" is mapped to "all values of the enum". If "ignoreUnknown" is true then when parsing, unknown values are ignored. This is recommended for forward compatiblity with later versions. 2. This support is implemented in org.apache.hadoop.fs.s3a.impl.ConfigurationHelper -it can be used elsewhere in the hadoop codebase. 3. A new private FlagSet class in hadoop common manages a set of enum flags. It implements StreamCapabilities and can be probed for a specific option being set (with a prefix) S3A adds an option fs.s3a.performance.flags which builds a FlagSet with enum type PerformanceFlagEnum * which initially contains {Create, Delete, Mkdir, Open} * the existing fs.s3a.create.performance option sets the flag "Create". * tests which configure fs.s3a.create.performance MUST clear fs.s3a.performance.flags in test setup. Future performance flags are planned, with different levels of safety and/or backwards compatibility. Contributed by Steve Loughran
steveloughran
added a commit
that referenced
this pull request
Jul 29, 2024
…performance flags (#6789) (#6966) 1. Configuration adds new method `getEnumSet()` to get a set of enum values from a configuration string. <E extends Enum<E>> EnumSet<E> getEnumSet(String key, Class<E> enumClass, boolean ignoreUnknown) Whitespace is ignored, case is ignored and the value "*" is mapped to "all values of the enum". If "ignoreUnknown" is true then when parsing, unknown values are ignored. This is recommended for forward compatiblity with later versions. 2. This support is implemented in org.apache.hadoop.fs.s3a.impl.ConfigurationHelper -it can be used elsewhere in the hadoop codebase. 3. A new private FlagSet class in hadoop common manages a set of enum flags. It implements StreamCapabilities and can be probed for a specific option being set (with a prefix) S3A adds an option fs.s3a.performance.flags which builds a FlagSet with enum type PerformanceFlagEnum * which initially contains {Create, Delete, Mkdir, Open} * the existing fs.s3a.create.performance option sets the flag "Create". * tests which configure fs.s3a.create.performance MUST clear fs.s3a.performance.flags in test setup. Future performance flags are planned, with different levels of safety and/or backwards compatibility. Contributed by Steve Loughran
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
…performance flags (apache#6789) 1. Configuration adds new method `getEnumSet()` to get a set of enum values from a configuration string. <E extends Enum<E>> EnumSet<E> getEnumSet(String key, Class<E> enumClass, boolean ignoreUnknown) Whitespace is ignored, case is ignored and the value "*" is mapped to "all values of the enum". If "ignoreUnknown" is true then when parsing, unknown values are ignored. This is recommended for forward compatiblity with later versions. 2. This support is implemented in org.apache.hadoop.fs.s3a.impl.ConfigurationHelper -it can be used elsewhere in the hadoop codebase. 3. A new private FlagSet class in hadoop common manages a set of enum flags. It implements StreamCapabilities and can be probed for a specific option being set (with a prefix) S3A adds an option fs.s3a.performance.flags which builds a FlagSet with enum type PerformanceFlagEnum * which initially contains {Create, Delete, Mkdir, Open} * the existing fs.s3a.create.performance option sets the flag "Create". * tests which configure fs.s3a.create.performance MUST clear fs.s3a.performance.flags in test setup. Future performance flags are planned, with different levels of safety and/or backwards compatibility. Contributed by Steve Loughran
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
…performance flags (apache#6789) 1. Configuration adds new method `getEnumSet()` to get a set of enum values from a configuration string. <E extends Enum<E>> EnumSet<E> getEnumSet(String key, Class<E> enumClass, boolean ignoreUnknown) Whitespace is ignored, case is ignored and the value "*" is mapped to "all values of the enum". If "ignoreUnknown" is true then when parsing, unknown values are ignored. This is recommended for forward compatiblity with later versions. 2. This support is implemented in org.apache.hadoop.fs.s3a.impl.ConfigurationHelper -it can be used elsewhere in the hadoop codebase. 3. A new private FlagSet class in hadoop common manages a set of enum flags. It implements StreamCapabilities and can be probed for a specific option being set (with a prefix) S3A adds an option fs.s3a.performance.flags which builds a FlagSet with enum type PerformanceFlagEnum * which initially contains {Create, Delete, Mkdir, Open} * the existing fs.s3a.create.performance option sets the flag "Create". * tests which configure fs.s3a.create.performance MUST clear fs.s3a.performance.flags in test setup. Future performance flags are planned, with different levels of safety and/or backwards compatibility. Contributed by Steve Loughran
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HADOOP-19161
Initial design
For testing we need to make sure ths is unset from all cost tests.
relates to #6543; the logic to set up that operation is here...that PR would
just be the implementation.
Same for a delete optimisation where we'd skip parent dir probe.
rename could do the same for its source dir too.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?