Skip to content

Conversation

@kombucha
Copy link
Contributor

@kombucha kombucha commented Nov 14, 2023

Q A
Bug fix? yes
New feature? no
BC breaks? ?
Related Issue N/A
Need Doc update no

Describe your change

Use the host.url, which should be unique, instead of the host.accept value, which in most cases is not unique, when caching the http client in the @connections instance variable of http_requester

What problem is this fixing?

Failing to do that, all hosts will actually use the same http client as the first one (assuming they share the same accept value). This prevents the retry strategy from working properly
For example:

  1. you have host1, host2 & host3 configured. host1 is down.
  2. you call algolia_client.indexes
  3. transporter -> retry_strategy -> http_requester will try to request host1 and instantiate an http client with its url.
  4. The request fails
  5. There's a retry on host2, but http_requester.connections[host2.accept] will yield the same http client as host1!
  6. All subsequent requests will fail despite the retry strategy

@kombucha kombucha added the Bug label Nov 14, 2023
@kombucha kombucha requested a review from DevinCodes November 14, 2023 12:23
@kombucha kombucha self-assigned this Nov 14, 2023
@kombucha kombucha force-pushed the fix-http-requester-connections-cache-key branch 3 times, most recently from fec709a to a249bfb Compare November 14, 2023 13:33
@kombucha kombucha marked this pull request as ready for review November 14, 2023 13:35
DevinCodes
DevinCodes previously approved these changes Nov 14, 2023
@kombucha kombucha force-pushed the fix-http-requester-connections-cache-key branch 2 times, most recently from 17ec156 to cb6cfe3 Compare November 14, 2023 13:44
@kombucha kombucha force-pushed the fix-http-requester-connections-cache-key branch from cb6cfe3 to 0a2694f Compare November 14, 2023 13:50
@DevinCodes DevinCodes merged commit 7f2672d into master Nov 14, 2023
@DevinCodes DevinCodes deleted the fix-http-requester-connections-cache-key branch November 14, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants