Repeat API calls on timeouts #358
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
azawlocki
changed the title
azawlocki
changed the title
azawlocki
commented
Apr 30, 2021
| try: | ||
| proposals = await self._api.collect_offers(self._id, timeout=10, max_events=10) | ||
| async with SuppressedExceptions(is_recoverable_exception): | ||
| proposals = await self._api.collect_offers(self._id, timeout=5, max_events=10) |
There was a problem hiding this comment.
Wrapping this call in SuppressedExceptions(is_recoverable_exception) would ignore timeout errors and aiohttp.ServerDisconnectedError.
|
Awesome fixes! 🥳 FYI the core team needs to double check if the re-tries are handled properly before releasing.. |
filipgolem
approved these changes
May 6, 2021
azawlocki
force-pushed
the
az/repeat-invoice-accept
branch
from
9825e90 to
fdca644
Compare
filipgolem
reviewed
May 14, 2021
Comment on lines
+73
to
74
| @repeat_on_error(max_tries=5) | ||
| async def details(self) -> AllocationDetails: |
There was a problem hiding this comment.
I think that this method is never called, so there is no need to add @repeat_on_error here.
|
I tested these changes with yagna daemon. Internal timeouts values were random, so that sometimes calls fail and sometimes succeed. |
nieznanysprawiciel
approved these changes
May 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes in this PR:
Executorretry some operations that raise errors that indicate timeouts in API calls:Invoice.accept()DebitNote.accept()collect_offers()get_invoice_events()get_debit_note_events()The "timeout errors" are HTTP errors 408 and 504,
asyncio.TimeoutError(indicating client-side timeout) and intermittent errorsaiohttp.ClientDisconnectedErrorandaiohttp.ClientOSError(errno=32)(Broken Pipe).Should fix:
Note: "should fix" is obviously an overstatement, this PR won't prevent timeout-related API errors, it merely repeats each failed API call few times, making an assumption that the more attempts we make the less likely is that they will all fail.