Transport class does not handle retry properly.

[teod-sh]

Description:

Retry does not work as expected. Every time a retry condition comes, the code will just raise an exception.

Items with the issue:

• Async transporter: https://github.com/algolia/algoliasearch-client-python/blob/main/algoliasearch/http/transporter.py
• Sync transporter: https://github.com/algolia/algoliasearch-client-python/blob/main/algoliasearch/http/transporter_sync.py

Side-affected items:

• All modules that call Algolia servers from the algoliasearch-client-python lib
• algoriasearch_django lib on https://github.com/algolia/algoliasearch-django/blob/master/algoliasearch_django/registration.py

Fix Suggestion: Consider a retry flag, implement retry with exponential waiting time, so clients won’t make the life of the servers even harder when recovering from some outage, or just keep the loop going to the next server so the retry will target the next host, avoiding exponential waiting.
If retry will be really in place, it may be a good idea to have some value to manage it, on/off, retry after etc…

[shortcuts]

Hey @teod-sh thanks for opening the issue, I just tried locally and can confirm the client retries properly when hosts are unreachable (https://github.com/algolia/algoliasearch-client-python/blob/main/algoliasearch/http/retry.py#L44), so if for example an host takes too long to answer, the request will be retried, however if the request is incorrect (e.g. wrong credentials, index does not exist), it will stop there

would you mind providing a reproduction with your client implementation and/or what you'd expect to be retried?

[teod-sh]

Hi @shortcuts
I don't have any working test for it, I was just walking around and found it.

About the retry class, it detects well when should retry, that's fine, what I have pointed is more related to this section:

    async def request(OMMITED) -> ApiResponse:
       # ommited

        # loop through hosts
        for host in self._retry_strategy.valid_hosts(self._hosts):
            url = self.build_url(host, path)
            proxy = self.get_proxy(url)

            try:
                 # tries request
                async with timeout(self._timeout / 1000):
                    resp = await self._session.request(
                        method=verb,
                        url=url,
                        headers=request_options.headers,
                        data=request_options.data,
                        proxy=proxy,
                    )

                    _raw_data = await resp.text()
                    response = ApiResponse(
                            #....ommited
                    )

            except TimeoutError as e:
                response = ApiResponse(
                    verb=verb,
                    path=path,
                    url=url,
                    host=host.url,
                    error_message=str(e),
                    is_timed_out_error=True,
                )

            # decide if retry should be triggered
            decision = self._retry_strategy.decide(host, response)

            if decision == RetryOutcome.SUCCESS:
                # for a 500 status error you get a RetryOutcome.SUCCESS, so
                # it will return a response
                return response
            #... ommited

I can be wrong too, again I was just walking around to see the code.

[shortcuts]

You only get a SUCCESS in that case https://github.com/algolia/algoliasearch-client-python/blob/main/algoliasearch/http/retry.py#L41-L42, so for a 2xx status code, anything would fall under a FAIL outcome otherwise

[teod-sh]

Yep, you are right, and when the retry status arrives, it will loop to the next host.
It was a misunderstanding on my side, sorry about that :)

Thanks, @shortcuts !

[shortcuts]

no problem!