-
Notifications
You must be signed in to change notification settings - Fork 19
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
Introduce ZYTE_API_MAX_REQUESTS #114
Conversation
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 89.48% 98.49% +9.00%
==========================================
Files 9 9
Lines 647 666 +19
==========================================
+ Hits 579 656 +77
+ Misses 68 10 -58
|
|
||
if self._max_requests_reached(downloader): | ||
self._crawler.engine.close_spider(spider, "closespider_max_zapi_requests") | ||
raise IgnoreRequest("Reached max Zyte API requests") |
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.
As, I think, this code will be executed several times, I wonder how will it look in the log, hopefully not too ugly.
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.
There should just be a few of them since the spider is immediately closed.
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.
As long as the message reads “this request is skipped because the max has been reached” rather than just “the max has been reached”, I think it is OK for this to appear several times.
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.
Message updated in 660cdb2
I think the approach is fine, though I haven't thought about the |
fa93be0
to
63a9d44
Compare
Hey folks, I've unmarked this one as draft. I think we can think about other things to set as max like item count, per data type, per page type, etc in another future PR. |
Co-authored-by: Adrián Chaves <[email protected]>
Thanks @BurnzZ! |
An attempt on #111 to gather feedback.
This has minimal changes that only addresses the limits on ZAPI Requests.
In the future, we also might want to impose other limits like the following which may affect the implementation:
TODO