-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Cache requests in RQ implementations by id
#633
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
base: master
Are you sure you want to change the base?
Conversation
288e0b3
to
d3dab06
Compare
d3dab06
to
922a4c8
Compare
59a33b1
to
4209d75
Compare
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.
a few comments
logger.debug( | ||
'Cannot find a request from the beginning of queue, will be retried later', | ||
extra={'nextRequestUniqueKey': next_unique_key}, | ||
extra={'nextRequestId': next_request_id}, |
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.
why camelCase?
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.
Maybe you can answer that question 😉 , I was just following the same pattern you started:
https://github.com/apify/apify-sdk-python/pull/470/files#diff-82643bb1c4a8c0e5e3b8044d2248476e4c8e36fbff7bf672cbacd10368e17357R314
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.
Haha, yeah... mistake on my part; so could you please fix it since you're updating it?
logger.debug( | ||
'Request fetched from the beginning of queue was already handled', | ||
extra={'nextRequestUniqueKey': next_unique_key}, | ||
extra={'nextRequestId': next_request_id}, |
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.
why camelCase?
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.
Same as above
response = await self._api_client.get_request(unique_key_to_request_id(unique_key)) | ||
return await self._get_request_by_id(unique_key_to_request_id(unique_key)) | ||
|
||
async def _get_request_by_id(self, request_id: str) -> Request | 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.
Methods order please (https://pypi.org/project/flake8-class-attributes-order/)
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.
I am reluctant to follow a formatting rule that is not enforced automatically by a tool. We do not even have a coding style defined, so how does one know what rule to follow? The code base is not compliant with the rule anyway. Cherry picking it randomly in some PR's is just annoying.
I will be happy to follow the rule as long as it is enforced by a tool.
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.
src/apify/storage_clients/_apify/_request_queue_shared_client.py
Outdated
Show resolved
Hide resolved
src/apify/storage_clients/_apify/_request_queue_single_client.py
Outdated
Show resolved
Hide resolved
return self._requests_cache[unique_key] | ||
return await self._get_request(id=unique_key_to_request_id(unique_key)) | ||
|
||
async def _get_request(self, id: str) -> Request | 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.
Methods order please (https://pypi.org/project/flake8-class-attributes-order/)
Description
id
instead ofunique_key
due to the Apify platform truncating long unique_keys.Issues
list_head
method returns truncated unique keys and URLs for long URLs #630Testing