-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-18845. Add ability to configure s3 connection ttl #5948
Conversation
Ran tests again us-west-1. All good. |
💔 -1 overall
This message was automatically generated. |
@@ -154,6 +156,11 @@ private Constants() { | |||
public static final String MAXIMUM_CONNECTIONS = "fs.s3a.connection.maximum"; | |||
public static final int DEFAULT_MAXIMUM_CONNECTIONS = 96; | |||
|
|||
// Expiration time of s3 http connection from the connection pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make javadoc; and use @value
@@ -154,6 +156,11 @@ private Constants() { | |||
public static final String MAXIMUM_CONNECTIONS = "fs.s3a.connection.maximum"; | |||
public static final int DEFAULT_MAXIMUM_CONNECTIONS = 96; | |||
|
|||
// Expiration time of s3 http connection from the connection pool. | |||
// See {@code com.amazonaws.ClientConfiguration#setConnectionTTL} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cut the comment... v2 upgrade obsolete.
// Expiration time of s3 http connection from the connection pool. | ||
// See {@code com.amazonaws.ClientConfiguration#setConnectionTTL} | ||
public static final String CONNECTION_TTL = "fs.s3a.connection.ttl"; | ||
public static final long DEFAULT_CONNECTION_TTL = ClientConfiguration.DEFAULT_CONNECTION_TTL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set the default to 5 mins.
@@ -1782,6 +1782,23 @@ will attempt to retry the operation; it may just be a transient event. If there | |||
are many such exceptions in logs, it may be a symptom of connectivity or network | |||
problems. | |||
|
|||
Above error could be because of a stale http connections. By default, connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*the above.
@@ -1782,6 +1782,23 @@ will attempt to retry the operation; it may just be a transient event. If there | |||
are many such exceptions in logs, it may be a symptom of connectivity or network | |||
problems. | |||
|
|||
Above error could be because of a stale http connections. By default, connections | |||
in the http connection pool are reused indefinitely. To discard connections after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new default is 5 mins.
Expiration time for a connection in the connection pool in milliseconds. | ||
When a connection is retrieved from the connection pool, | ||
this parameter is checked to see if the connection can be reused. | ||
Default value is set to -1 (infinite) which means connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default in sdk is -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed in a conf call; you've noted my feedback.
Steve's review comments
🎊 +1 overall
This message was automatically generated. |
public static final long DEFAULT_CONNECTION_TTL = ClientConfiguration.DEFAULT_CONNECTION_TTL; | ||
|
||
/** | ||
* Default value for {@value CONNECTION_TTL}: {@value}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will convert it to the actual value so I think it is good https://javadoc.io/doc/org.apache.hadoop/hadoop-aws/latest/index.html
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/troubleshooting_s3a.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/troubleshooting_s3a.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/troubleshooting_s3a.md
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to go in, had some doubts.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Invoker.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments on tests.
one bigger issue: why do we support a TTL with granularity <1s? as most other units are in seconds and it'd be good to be consistent.
(using getFloat() and fractions of a second is probably too clever)
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
+1
….connection.ttl (#5948) Contributed By: Mukund Thakur
merged to trunk and branch-3.3. Thanks for reviews. |
….connection.ttl (apache#5948) Contributed By: Mukund Thakur
Description of PR
Introducing fs.s3a.connection.ttl configuration which can be configured to expire old HTTP connection after specified period of time.
How was this patch tested?
Tests in progress. Added a new test.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?