Expose additional configuration options for Http connection manager #204
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
TingDaoK
reviewed
Aug 22, 2022
waahm7
commented
Aug 25, 2022
waahm7
changed the title
graebm
reviewed
Aug 26, 2022
include/aws/s3/s3_client.h
Outdated
| struct aws_s3_tcp_keep_alive_options { | ||
|
|
||
| /* Set keepalive true to periodically transmit messages for detecting a disconnected peer.*/ | ||
| bool keepalive; |
There was a problem hiding this comment.
do we need bool here? wouldnt null keep alive indicate keep alive is on?
There was a problem hiding this comment.
Do you mean null keep alive will indicate that keep alive is off? Since it is just a normal struct, it can't be null. We can turn it into a pointer and remove the keepalive. If the pointer is null, that would mean keepalive is off and vice versa.
Otherwise, we need a keepalive variable so that users can just set keepalive to true and use default values for the rest. This is basically a partial struct from aws_socket_options which has two extra fields enum aws_socket_type type and enum aws_socket_domain domain. I didn't want to allow users to override these two fields.
waahm7
commented
Aug 29, 2022
Comment on lines
+318
to
+324
| if (client_config->proxy_ev_settings->tls_options) { | ||
| client->proxy_ev_tls_options = aws_mem_calloc(allocator, 1, sizeof(struct aws_tls_connection_options)); | ||
| if (aws_tls_connection_options_copy(client->proxy_ev_tls_options, client->proxy_ev_settings->tls_options)) { | ||
| goto on_error; | ||
| } | ||
| client->proxy_ev_settings->tls_options = client->proxy_ev_tls_options; | ||
| } |
There was a problem hiding this comment.
Is there a better way to deep copy proxy_ev_settings->tls_options? The problem is that in proxy_ev_settings, tls_options is declared as const. So if I just use that directly instead of using client->proxy_ev_tls_options, I get a bunch of warnings about passing const tls_options to other functions which expect a non-const object like aws_mem_release.
waahm7
commented
Aug 29, 2022
Comment on lines
+307
to
+312
| if (client_config->proxy_options) { | ||
| client->proxy_config = aws_http_proxy_config_new_from_proxy_options(allocator, client_config->proxy_options); | ||
| if (client->proxy_config == NULL) { | ||
| goto on_error; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am unable to write a test for the proxy_config override. As the struct proxy_config implementation detail is private in the aws-c-http repo, I can't access its members to assert.
graebm
reviewed
Aug 29, 2022
waahm7
changed the title
DmitriyMusatkin
approved these changes
Aug 31, 2022
grrtrr
added a commit
to grrtrr/aws-sdk-cpp
that referenced
this pull request
Sep 21, 2022
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.
This was referenced Sep 21, 2022
jmklix
pushed a commit
to aws/aws-sdk-cpp
that referenced
this pull request
Oct 25, 2022
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.
11 tasks
jmklix
added a commit
to aws/aws-sdk-cpp
that referenced
this pull request
Oct 28, 2022
* [aws-cpp-sdk-s3-crt]: Configure TCP connection-monitoring options 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. * Use stack-allocated rather than dynamically allocated variables * add code gen * use int instead of uint16_t and uint32_t * use static_cast * rework static cast * add another static cast * remove static cast and use unsigned long Co-authored-by: Gerrit Renker <grenker@aurora.tech>
jmklix
added a commit
to aws/aws-sdk-cpp
that referenced
this pull request
Nov 1, 2022
* [aws-cpp-sdk-s3-crt]: Configure TCP connection-monitoring options 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. * Use stack-allocated rather than dynamically allocated variables * add code gen * use int instead of uint16_t and uint32_t * use static_cast * rework static cast * add another static cast * remove static cast and use unsigned long Co-authored-by: Gerrit Renker <grenker@aurora.tech>
jmklix
added a commit
to aws/aws-sdk-cpp
that referenced
this pull request
Aug 11, 2023
* [aws-cpp-sdk-s3-crt]: Configure TCP connection-monitoring options 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. * Use stack-allocated rather than dynamically allocated variables * add code gen * use int instead of uint16_t and uint32_t * use static_cast * rework static cast * add another static cast * remove static cast and use unsigned long Co-authored-by: Gerrit Renker <grenker@aurora.tech>
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.
Issue #, if available: awslabs/aws-crt-java#502 & aws/aws-sdk-cpp#1882
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.