-
Notifications
You must be signed in to change notification settings - Fork 5
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
818 stac requests resilience #1022
base: master
Are you sure you want to change the base?
Conversation
Due to a bug in the original StacAPIIo class we were forced to create a custom implementation (stac-utils/pystac-client#706). See #818 for more information.
Given that we are using urllib3.Poolmanager.request instead of urllib3.request this required a different patch.
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.
some quick notes
|
||
|
||
class StacApiIO(DefaultStacIO): | ||
|
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.
Can you document what this class adds on top of the existing DefaultStacIO?
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.
Done.
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.
While checking some pystac details I stumbled on pystac.stac_io.RetryStacIO
https://pystac.readthedocs.io/en/stable/api/stac_io.html#pystac.stac_io.RetryStacIO shouldn't we just use that instead?
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 discussed: RetryStacIO does not have timeout control.
I opened a ticket to feature-request that already:
I would add a technical debt comment to migrate to that solution in the future instead of having to maintain this home-grown wrapper/hack
tests/test_api_result.py
Outdated
|
||
def get(self, href, data): | ||
code = 200 | ||
self.urllib_mock.get(href, data, code) | ||
if isinstance(data, str): | ||
data = data.encode("utf-8") | ||
self.requests_mock.get(href, content=data) | ||
|
||
self.urlopen_mocker.get(href, data, code) |
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.
data
is bytes here, is that ok?
|
||
|
||
class StacApiIO(DefaultStacIO): | ||
|
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.
While checking some pystac details I stumbled on pystac.stac_io.RetryStacIO
https://pystac.readthedocs.io/en/stable/api/stac_io.html#pystac.stac_io.RetryStacIO shouldn't we just use that instead?
I use a Poolmanager for the retry mechanism as stated in the urllib3 docs:
https://urllib3.readthedocs.io/en/stable/user-guide.html#making-requests
This did require me to create an extra mock class for Poolmanager.request.