8000 [HttpClient] Allow using multiple base_uri as array for retries by Tiriel · Pull Request #49809 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpClient] Allow using multiple base_uri as array for retries #49809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2023
Merged

[HttpClient] Allow using multiple base_uri as array for retries #49809

merged 1 commit into from
Apr 19, 2023

Conversation

Tiriel
Copy link
Contributor
@Tiriel Tiriel commented Mar 25, 2023 •
edited by nicolas-grekas
Loading
Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #43327
License MIT
Doc PR TBD

This pull request enhances the RetryableHttpClient by allowing the use of an array of URLs as the base_uri option. This enables retries on different base URIs for each retry attempt, providing increased reliability and flexibility for making HTTP requests.

Basic usage :

use Symfony\Component\HttpClient\RetryableHttpClient;

$client = new RetryableHttpClient(
    HttpClient::create(),
    new GenericRetryStrategy([500], 0),
    1
);

$response = $client->request('GET', 'foo-bar', [
    'base_uri' => [
        'http://example.com/a/', // first request will use this base uri
        'http://example.com/b/', // if first request fails, the second base uri will be used
    ],
]);

Usage with base uri shuffling :

use Symfony\Component\HttpClient\RetryableHttpClient;

$client = new RetryableHttpClient(
    HttpClient::create(),
    new GenericRetryStrategy([500], 0),
    3
);

$response = $client->request('GET', 'foo-bar', [
    'base_uri' => [
        'http://example.com/a/',
        [
            'http://example.com/b/', // base uri passed inside a nested array will be shuffled
            'http://example.com/c/',
        ]
        'http://example.com/d/', // the order of the non-nested base uris is preserved
    ],
]);

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@Tiriel Tiriel marked this pull request as ready for review March 25, 2023 13:32
@carsonbot carsonbot added this to the 6.3 milestone Mar 25, 2023
@Tiriel Tiriel changed the title [HttpClient] [WiP] Allow using multiple base_uri as array for retries [HttpClient] Allow using multiple base_uri as array for retries Mar 28, 2023
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 28, 2023 •
edited
Loading

ghanks for starting this. We should also support default options, as is possible when using $retryable->withOptions().

About what you say @GromNaN, it should also be possible to pass a list of per-hosts IPs to the resolve option, and rotate them as fits. This could be done either in this PR if @Tiriel is up to, or in a later one. Of course, that'd need resolving the many IPs outside of the client.

One thing I'm wondering in both cases: shouldn't we shuffle the base URIs (and IPs later on?)

@Tiriel
Copy link
Contributor Author
Tiriel commented Mar 28, 2023

@nicolas-grekas Sure thing, going to work on the default options!
For you last question, I wonder if it would be a good idea to leave it open and give an option. Like, if base_uri is an array, you can pass a 'shuffle' => true option defaulting to false, otherwise it doesn't do anything? Or maybe it's too much? Because I can clearly see use cases where you wouldn't want to shuffle, and some people seem to have use cases for the contrary.

As for the IPs, I thought it would be better in another PR since the resolve doesn't take place at the same level, but maybe we can work something out. So I'd say that's up to @GromNaN , if you want to work on the feature yourself or not.

@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 12, 2023

Made an attempt to make it work with withOptions, it's working, but quite frankly I'm not sure if my code is ok or "dirty", so any advice would be appreciated.

Haven't tried yet on the shuffling part, I'd like opinion on wether to shuffle by default or not, or on adding an option for this.

@nicolas-grekas
Copy link
Member

About shuffling, we could to it this way:

$hosts = ['h1', 'h2', ['h3', h4']]

Nested arrays would define shuffle groups, and first level values would be used in order.
Up for implementing this?

@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 13, 2023

That's a great idea, thanks!
Will work on it asap!

@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 19, 2023

Forgot to ping, but this has been updated with a shuffling attempt and tests accordingly.

nicolas-grekas
nicolas-grekas approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified a bit the implementation, see https://github.com/symfony/symfony/compare/90b2027189ff18ca4957567e2252f570e3276d6d..b63ad4025ffbc4f1e513dd34650a34a50712afe4

I also removed the exception when the URL is absolute and a base URI is still provided because this is not an erroneous situation.

Lastly, I ensured that if the number of retries is higher than the number of base URIs, we stick retrying to the last one (possibly randomizing from a list).

@nicolas-grekas
Copy link
Member

Thank you @Tiriel.

@nicolas-grekas nicolas-grekas mentioned this pull request Apr 19, 2023
Closed
@nicolas-grekas nicolas-grekas merged commit 33da4c8 into symfony:6.3 Apr 19, 2023
@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 19, 2023

Looks more than a bit simplified to me, this is awesome work to look at and learn from! Thanks!

@lyrixx
Copy link
Member
lyrixx commented Apr 19, 2023

@Tiriel Can you update the PR description with some code, please? It'll help Javier to write the living-on-the-edge articles

@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 19, 2023

@lyrixx Will do this evening!

@Tiriel
Copy link
Contributor Author
Tiriel commented Apr 19, 2023

Updated PR description with usage, let me know if more is needed!

@Tiriel Tiriel mentioned this pull request Apr 20, 2023
Merged
Tiriel
Tiriel commented Apr 20, 2023
View reviewed changes
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Apr 20, 2023
@OskarStark
feature #18227 [HttpClient] Add doc for multiple base_uri as array …
c63db2b 
…(Tiriel)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[HttpClient] Add doc for multiple `base_uri` as array

Add documentation for multiple base_uris in HttpClient (Symfony PR (merged): [symfony/symfony#49809](symfony/symfony#49809) )
Fixes #18224

Commits
-------

7244375 [HttpClient] Add doc for multiple `base_uri` as array
@ptica
Copy link
ptica commented Dec 18, 2023

hi, thank you for the feature!
there is still a validation that prevents configuring the base_uri as an array in framework.yaml config,
is that intentional?

https://github.com/symfony/symfony/blob/7.1/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L1861

Screenshot 2023-12-18 at 15 58 46

@Tiriel
Copy link
Contributor Author
Tiriel commented Dec 18, 2023

Hi! Thanks for the feedback!

is that intentional?

Not on my part. Going to write a fix for this asap!

@Tiriel Tiriel mentioned this pull request Dec 18, 2023
Closed
@Tiriel Tiriel mentioned this pull request Dec 18, 2023
Open
Tiriel added a commit to Tiriel/symfony that referenced this pull request Dec 19, 2023
@Tiriel
[FrameworkBundle] Fix config for array of base_uri in http_client
83ce0a9 
Allow array of `base_uri` in config for HttpClient keys, as per
[comment in PR](symfony#49809 (comment)).
Tiriel added a commit to Tiriel/symfony that referenced this pull request Apr 25, 2024
@Tiriel
[FrameworkBundle] Fix config for array of base_uri in http_client
13afd2b 
Allow array of `base_uri` in config for HttpClient keys, as per
[comment in PR](symfony#49809 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx lyrixx left review comments

Assignees
No one assigned
Labels
Feature HttpClient Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

[HttpClient] Add support for client pool
5 participants
@Tiriel @carsonbot @nicolas-grekas @lyrixx @ptica
0