Rename property key alluxio.underfs.s3.threads.max #18670
Merged
Conversation
This file contains 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
Xenorith
approved these changes
Aug 8, 2024
Xenorith
added
the
type-ease-of-use
|
alluxio-bot, force-merge this please |
Xenorith
pushed a commit
that referenced
this pull request
Aug 9, 2024
### What changes are proposed in this pull request?
Rename property key alluxio.underfs.s3.threads.max to alluxio.underfs.s3.connections.max and bump the default number to 1024
### Why are the changes needed?
The property `alluxio.underfs.s3.threads.max` has a misleading name, as it actually represents the size of a connection pool rather than the max number of threads. Consequently, the value of this property is typically much larger (e.g., Trino uses 256 for the equivalent property to ensure sufficient bandwidth when reading from S3).
However, including "threads" in the name causes some users to worry, as they believe it should be calculated based on the number of vCores (as we found in one PoC that users were unwilling to set this value large) .
This commit changes the name but retains the original property name as an alias to maintain backward compatibility.
### Does this PR introduce any user facing changes?
Yes but backwards compatible
pr-link: #18670
change-id: cid-ae3e560225ca3f9d624bf321e75863f5834174b8
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes are proposed in this pull request?
Rename property key alluxio.underfs.s3.threads.max to alluxio.underfs.s3.connections.max and bump the default number to 1024
Why are the changes needed?
The property
alluxio.underfs.s3.threads.maxhas a misleading name, as it actually represents the size of a connection pool rather than the max number of threads. Consequently, the value of this property is typically much larger (e.g., Trino uses 256 for the equivalent property to ensure sufficient bandwidth when reading from S3).However, including "threads" in the name causes some users to worry, as they believe it should be calculated based on the number of vCores (as we found in one PoC that users were unwilling to set this value large) .
This commit changes the name but retains the original property name as an alias to maintain backward compatibility.
Does this PR introduce any user facing changes?
Yes but backwards compatible