-
Notifications
You must be signed in to change notification settings - Fork 159
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.
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
Fix/requests error handling #477
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks excellent! +1 for removing the tuples. pls. see one proposed improvement in the review.
what is missing is a short explanation how the timeout and retries are configured. please add a new section to performance.md
in the docs - give an example how to configure retries and what are the defaults
@@ -146,27 +148,29 @@ class Client: | |||
""" | |||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to decorate this @with_config(RuntimeConfiguration). and the right arguments will be injected. why I think it is cool:
- if someone creates a session from a source, the configuration may be defined in the scope of the source ("sources", "pipedrive", "runtime")
- maybe if at some point we'll do a lazy instantiation of default client, even the default config will be scoped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Added this and some docs!
d0978ca
to
60b4a46
Compare
60b4a46
to
cf0734f
Compare
Fixes #473
ChunkedLoadingException
added to retry list. Seems this almost always due to some kind of dropped connectionTimeout
andConnectionError
are already retried and all server errors inherit thoseRunConfiguration