-
Notifications
You must be signed in to change notification settings - Fork 186
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
[experiment] Add resource time limit and rate limiting #1485
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
if max_items >= 0 or max_time and not self.incremental: | ||
from dlt.common import logger | ||
|
||
logger.warning( |
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.
this will need to be improved a bit, but I think this is a quite nice solution.
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.
also: generally speaking it would be cool to have some wrapper around log messages to be able to test them, maybe mocking would be enough, not sure
while (last_iteration + min_wait) - time.time() > 0: | ||
# we give control back to the pipe iterator | ||
yield None | ||
time.sleep(0.1) |
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 could make this configurable, I am not sure wether it is needed though.
@@ -788,52 +788,6 @@ def test_add_transformer_right_pipe() -> None: | |||
iter([1, 2, 3]) | dlt.resource(lambda i: i * 3, name="lambda") | |||
|
|||
|
|||
def test_limit_infinite_counter() -> None: |
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.
these three tests were moved to the new location where a couple more tests will be added specifically for the limits
2ee3eab
to
e48f641
Compare
6126895
to
5fc2324
Compare
🎉 I like where this is going. What are your thoughts on how this could be used in conjunction with headers returned from API endpoints? which returns headers in each response: It basically allows you to adjust your requests based on a api key/user and an org-wide limit. The current rest client supports 429 responses, but my plan is to possibly preempt them as much as I can, so using these headers dynamically (e.g. update the throttling behavior after each response) would be my goal. |
Closing this PR in favor of: @joscha the api rate limiting things you have suggested would be a layer above in the rest_api implementation. There might already be a ticket for this or you could open a new one. |
Okay. How so, if it relies on a 429 answer or an explicit rest API resource returning the limits? Would you assume each resource to have some sort of hook to report back any time limits? |
Description
This PR extends the add_limit function to add time limits and rate limits to resources. This approach is to be discussed, but very straightforward, easy to test and works for both sync and async resources.
TODOs (if we go this route):
Other thoughts: