Connection Manager Acquisition Timeout #479
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
==========================================
- Coverage 79.43% 79.38% -0.05%
==========================================
Files 27 27
Lines 11622 11657 +35
==========================================
+ Hits 9232 9254 +22
- Misses 2390 2403 +13 ☔ View full report in Codecov by Sentry. |
waahm7
changed the title
source/connection_manager.c
Outdated
| uint64_t cull_task_time = s_calculate_idle_connection_cull_task_time(manager); | ||
| uint64_t connection_acquire_timeout = s_calculate_pending_connections_acquire_cull_task_time(manager); | ||
|
|
||
| return; | ||
| if (manager->max_connection_idle_in_milliseconds != 0 && manager->pending_connections_acquire_timeout_ms != 0) { | ||
| cull_task_time = aws_min_u64(cull_task_time, connection_acquire_timeout); | ||
| } else if (manager->pending_connections_acquire_timeout_ms != 0) { | ||
| cull_task_time = connection_acquire_timeout; | ||
| } |
There was a problem hiding this comment.
do we need two separate helper function to get the proper time for next cull task?
it seems more complicated than it needs to be.
There was a problem hiding this comment.
I started with one function, but it was more confusing since we need to cast the list node to different structs and check a different variable for full cull timestamp. It was the same amount of code duplication. So I split them into two separate functions.
source/connection_manager.c
Outdated
| /* | ||
| * Since the pending acquires are in FIFO order in the list, the front of the list has the closest | ||
| * cull time. | ||
| */ |
There was a problem hiding this comment.
it's just confusing that the comments above is
/*
* Since the connections are in LIFO order in the list, the front of the list has the closest
* cull time.
*/
And here, everything is the same but FIFO order to be mentioned...
I think for the acquisition it's not as complicated as the connections. It won't back to the queue after poped. So, it may be simpler to just mention that the front will be the closest cull time.
There was a problem hiding this comment.
Thanks, updated with the simple version.
source/connection_manager.c
Outdated
| AWS_FATAL_ASSERT(manager->cull_event_loop != NULL); | ||
|
|
||
| uint64_t cull_task_time = s_calculate_idle_connection_cull_task_time(manager); | ||
| uint64_t connection_acquire_timeout = s_calculate_pending_connections_acquire_cull_task_time(manager); |
There was a problem hiding this comment.
Keep the naming consistent with the existing naming. acquire -> acquisition
There was a problem hiding this comment.
Thanks, updated to what Mike suggested.
Co-authored-by: Dengke Tang <[email protected]>
Description of changes:
Introduce a new
pending_connections_acquire_timeout_mssimilar tomax_connection_idle_in_milliseconds. I have changed thes_schedule_connection_cullingto a generics_schedule_cullingwhich culls both the idle connections and pending acquisitions.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.