Skip to content

Retry requests - #83

Merged
tomasfejfar merged 10 commits into
masterfrom
feature-retry-request
Oct 17, 2024
Merged

tomasfejfar merged 10 commits into
masterfrom
feature-retry-request

Conversation

@Kukant

@Kukant Kukant commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@Kukant
Kukant requested review from odinuv and tomasfejfar October 11, 2024 11:37

@tomasfejfar tomasfejfar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's see what @odinuv

Comment thread kbcstorage/retry_requests.py Resolved
Comment thread kbcstorage/retry_requests.py Outdated
import time
import requests

MAX_RETRIES = 11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why 10 retries? That means 17 minutes request if the service is down completely.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we have 10 retries in SAPI, do we?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😱

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tak jsme to rozmotali, jak to je. Zásadní totiž je, že v php clientovi je to konfigurovatelné. Tím pádem tam, kde je to nějaký worker, který běží nekonečně je použitý default nebo i víc. A pak když je to používané někde v rámci requestu, tak je to zkrácené třeba na 3 pokusy. Tak prosím tady stejně. Tj. konfigurovatelné parametrem konstruktoru jako v té PHP verzi.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hotovson

@tomasfejfar

Copy link
Copy Markdown
Contributor

Also, while you're at it, please replace docker-compose with docker compose in the repo. The build is failing because of that.

@tomasfejfar tomasfejfar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM now, it will be battle tested when you use it :)

Comment thread tests/mocks/test_retry.py Resolved
@tomasfejfar
tomasfejfar merged commit c67dd40 into master Oct 17, 2024
@tomasfejfar
tomasfejfar deleted the feature-retry-request branch October 17, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants