Skip to content

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