[aws-cpp-sdk-s3-crt]: Configure TCP connection-monitoring options #2101
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
Based on awslabs/aws-c-s3#204, this exposes the following `ClientConfiguration` options to CRT: - `connect_timeout_ms`: S3 socket connection timeout; - `enableTcpKeepAlive`: enable TCP keep-alive probes; - `tcpKeepAliveIntervalMs`: TCP keep-alive wait/probe interval; - `lowSpeedLimit`: `aws-c-http` TCP connection speed monitoring; - `requestTimeoutMs`: to provide upper bound on the monitoring interval for `aws-c-http` TCP connection speed monitoring.
fboranek
reviewed
Oct 13, 2022
11 tasks
grrtrr
commented
Oct 26, 2022
| aws_s3_tcp_keep_alive_options tcp_keep_alive_options; | ||
| if (config.enableTcpKeepAlive) | ||
| { | ||
| const uint16_t keep_intvl = std::max<uint16_t>(15, config.tcpKeepAliveIntervalMs/1000); |
There was a problem hiding this comment.
@jmklix - I double-checked the aws-c-io/include/aws/io/socket.h, keep_alive_interval_sec takes an uint16_t.
Can you please help with details regarding failing tests - is is the std:max statement?
There was a problem hiding this comment.
The warning was warning C4244: '=': conversion from 'const int' to 'uint16_t', possible loss of data
and to fix that I changed it to this
uint16_t configKeepAliveS = static_cast<uint16_t>(std::min(static_cast<unsigned long>(std::numeric_limits<uint16_t>::max()), config.tcpKeepAliveIntervalMs / 1000ul));
static const uint16_t MAX_CRT_KEEP_ALIVE = 15; // seconds
const uint16_t keep_intvl = std::max(MAX_CRT_KEEP_ALIVE, configKeepAliveS);
grrtrr
commented
Oct 26, 2022
| const uint32_t monitor_intvl = std::max<uint32_t>(3, config.requestTimeoutMs/1000); | ||
|
|
||
| AWS_ZERO_STRUCT(tcp_monitoring_options); | ||
| tcp_monitoring_options.minimum_throughput_bytes_per_second = config.lowSpeedLimit; |
There was a problem hiding this comment.
Will use a static_cast here, lowSpeedLimit is unsigned long, while the LHS is uint64_t.
|
Changes have been merged here. Closing this PR |
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.
The main SDK enables TCP keep-alive and connection-speed monitoring by default, these options are also important for the S3CrtClient (e.g. awslabs/aws-c-s3#210).
Hence, based on awslabs/aws-c-s3#204, expose the following
ClientConfigurationoptions to CRT:connect_timeout_ms: S3 socket connection timeout;enableTcpKeepAlive: enable TCP keep-alive probes;tcpKeepAliveIntervalMs: TCP keep-alive wait/probe interval;lowSpeedLimit:aws-c-httpTCP connection speed monitoring;requestTimeoutMs: to provide upper bound on the monitoring interval foraws-c-httpTCP connection speed monitoring.Issue #, if available: Resolves #2107.
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.