-
Notifications
You must be signed in to change notification settings - Fork 9
Add support for OpenSearch API as ancillary data source #11
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: develop
Are you sure you want to change the base?
Conversation
jeremyh
left a comment
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.
Thanks Callum, looks very clean.
fetch/opensearch.py
Outdated
| Class for data retrievals using the OpenSearch API. | ||
| """ | ||
|
|
||
| def __init__(self, target_dir, api_url, username, password, query, show_progressbars=False, timeout=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.
We should set a default timeout, otherwise a network hiccup can leave the spawned task running indefinitely.
See the HTTP sources for an example of a hard-coded default timeout:
Line 245 in 6d4b6f7
| connection_timeout=DEFAULT_CONNECT_TIMEOUT_SECS): |
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.
(it's much more important on an ever-running service like fetch than on interactive scripts)
|
Looks good to me. As for a maintainer, there hasn't been much of any dedicated person. Just something that is |
|
Added default connection timeout to OpenSearchApiSource |
Hi @jeremyh @dunkgray @sixy6e
Not sure who's maintaining this one at the moment.
Sentinel-1 POEORB and RESORB ancillary recently switched to an OpenSearch API at Copernicus Hub. I've added some support to fetch for this API so we can continue to collect the ancillary.
Please take a look and see whether this is worth merging, and if there's any problems in the approach.