[PERF] Harden GCP Retries #3253
Conversation
samster25
commented
Nov 9, 2024
•
samster25
commented
- Introduces retries with exponential backoffs for GCS (default 5)
- Introduces connection and read timeouts (default 30 seconds)
- Introduces maximum connections for GCS (default 8/thread or 64)
- introduces idle connection clean up (max of 70)
CodSpeed Performance ReportMerging #3253 will improve performances by 28.43%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3253 +/- ##
==========================================
- Coverage 77.80% 77.66% -0.14%
==========================================
Files 645 645
Lines 79917 80056 +139
==========================================
Hits 62177 62177
- Misses 17740 17879 +139
|
| struct GCSClientWrapper(Client); | ||
| struct GCSClientWrapper { | ||
| client: Client, | ||
| connection_pool_sema: Arc<Semaphore>, |
There was a problem hiding this comment.
So IIUC this semaphore is:
- Acquired when we initiate a connection to GCS
- Released when the stream for that connection is exhausted?
For quick operations such as heads and stuff I guess we release it right after the result is obtained, hence the _permit pattern.
Could we add some short docstring here too describing that?
| Ok(IOConfig { | ||
| gcs: GCSConfig { | ||
| project_id, | ||
| credentials: credentials.map(|s| s.into()), | ||
| token, | ||
| anonymous: anonymous.unwrap_or(default.anonymous), | ||
| max_connections_per_io_thread: max_connections_per_io_thread |
There was a problem hiding this comment.
| max_connections_per_io_thread: max_connections_per_io_thread | |
| max_connections_per_io_thread |
Weird that lint didn't catch this
There was a problem hiding this comment.
the next line has an unwrap.
| .connect_timeout(Duration::from_millis(config.connect_timeout_ms)) | ||
| .read_timeout(Duration::from_millis(config.read_timeout_ms)) | ||
| .pool_idle_timeout(Duration::from_secs(60)) | ||
| .pool_max_idle_per_host(70) |
There was a problem hiding this comment.
Why 70? How many connections does it create anyways
There was a problem hiding this comment.
This is for idle connections for connection reuse. this is the default for many AWS SDKS
