Skip to content
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

Add support for OpenSearch API as ancillary data source #11

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

c-mckenna
Copy link
Member

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.

Copy link
Member

@jeremyh jeremyh left a 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.

Class for data retrievals using the OpenSearch API.
"""

def __init__(self, target_dir, api_url, username, password, query, show_progressbars=False, timeout=None,
Copy link
Member

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:

connection_timeout=DEFAULT_CONNECT_TIMEOUT_SECS):

Copy link
Member

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)

@sixy6e
Copy link

sixy6e commented Apr 10, 2021

Looks good to me.

As for a maintainer, there hasn't been much of any dedicated person. Just something that is update/fix when required. Might be worth raising this with @simonaoliver. Thoughts @dunkgray and @jeremyh?

@c-mckenna
Copy link
Member Author

Thanks @dunkgray @jeremyh @sixy6e,

Sorry completely missed the comments on this.

I'll push an update tomorrow addressing the timeout feedback from @jeremyh, thanks for that!

@c-mckenna
Copy link
Member Author

Added default connection timeout to OpenSearchApiSource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants