-
Notifications
You must be signed in to change notification settings - Fork 27
fix(harvester): recover records skipped by live pagination #886
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
|
|
||
| from cds_rdm.inspire_harvester.transform.resource_types import ALL_DOCUMENT_TYPES | ||
|
|
||
| INSPIRE_LITERATURE_API = "https://inspirehep.net/api/literature" | ||
|
|
||
|
|
||
| class InspireHTTPReader(BaseReader): | ||
| """INSPIRE HTTP Reader.""" | ||
|
|
@@ -40,40 +42,129 @@ def __init__( | |
|
|
||
| super().__init__(origin, mode, *args, **kwargs) | ||
|
|
||
| def _iter(self, url, *args, **kwargs): | ||
| def _build_url(self, q, **params): | ||
| """Build an INSPIRE literature search URL.""" | ||
| query_params = {"q": q, **params} | ||
| return f"{INSPIRE_LITERATURE_API}?{urlencode(query_params)}" | ||
|
|
||
| def _get_json(self, url, headers): | ||
| """Fetch JSON from INSPIRE or raise ReaderError.""" | ||
| current_app.logger.info(f"Querying URL: {url}.") | ||
| response = requests.get(url, headers=headers) | ||
| if response.status_code != 200: | ||
| error_message = ( | ||
| f"Error occurred while getting JSON data from INSPIRE. " | ||
| f"See URL: {url}. Error message: {response.text}. " | ||
| f"Status code: {response.status_code}" | ||
| ) | ||
| current_app.logger.error(error_message) | ||
| raise ReaderError(error_message) | ||
| current_app.logger.debug("Request response is successful (200).") | ||
| return response.json() | ||
|
|
||
| def _scan_ids(self, q, headers): | ||
| """Paginate an ID-only search and return IDs plus the reported total.""" | ||
| ids = set() | ||
| url = self._build_url(q, fields="id", size=1000) | ||
| reported_total = None | ||
|
|
||
| while url: | ||
| data = self._get_json(url, headers) | ||
| if reported_total is None: | ||
| reported_total = data["hits"]["total"] | ||
| for hit in data["hits"]["hits"]: | ||
| ids.add(str(hit["id"])) | ||
| url = data.get("links", {}).get("next") | ||
|
|
||
| return ids, reported_total | ||
|
|
||
| def _iter(self, url, q, *args, **kwargs): | ||
| """Yields HTTP response.""" | ||
| # header set to include additional data (external file URLs and more detailed metadata | ||
| headers = {"Accept": "application/vnd+inspire.record.expanded+json"} | ||
| initial_url = url | ||
| seen_ids = set() | ||
| expected_total = None | ||
| had_another_page = False | ||
|
|
||
| while url: # Continue until there is no "next" link | ||
| current_app.logger.info(f"Querying URL: {url}.") | ||
| response = requests.get(url, headers=headers) | ||
| data = response.json() | ||
| if response.status_code == 200: | ||
| current_app.logger.debug("Request response is successful (200).") | ||
| total = data["hits"]["total"] | ||
| hits = data["hits"]["hits"] | ||
|
|
||
| if total == 0: | ||
| current_app.logger.warning( | ||
| f"No results found when querying INSPIRE. See URL: {url}." | ||
| ) | ||
| elif url == initial_url: | ||
| current_app.logger.info(f"Records found: {total}.") | ||
|
|
||
| for inspire_record in hits: | ||
| current_app.logger.debug( | ||
| f"Sending INSPIRE record #{inspire_record['id']} to transformer." | ||
| ) | ||
| yield inspire_record | ||
| else: | ||
| error_message = f"Error occurred while getting JSON data from INSPIRE. See URL: {url}. Error message: {response.text}. Status code: {response.status_code}" | ||
| current_app.logger.error(error_message) | ||
| raise ReaderError(error_message) | ||
| data = self._get_json(url, headers) | ||
| total = data["hits"]["total"] | ||
| hits = data["hits"]["hits"] | ||
|
|
||
| if total == 0: | ||
| current_app.logger.warning( | ||
| f"No results found when querying INSPIRE. See URL: {url}." | ||
| ) | ||
| elif url == initial_url: | ||
| expected_total = total | ||
| current_app.logger.info(f"Records found: {total}.") | ||
|
|
||
| for inspire_record in hits: | ||
| record_id = str(inspire_record["id"]) | ||
| if record_id in seen_ids: | ||
| continue | ||
| seen_ids.add(record_id) | ||
| current_app.logger.debug( | ||
| f"Sending INSPIRE record #{record_id} to transformer." | ||
| ) | ||
| yield inspire_record | ||
|
|
||
| # Get the next page URL if available | ||
| url = data.get("links", {}).get("next") | ||
| # Remember if there was a second page. | ||
| if url: | ||
| had_another_page = True | ||
|
|
||
| # If results moved while we were paging, we may have missed some records. | ||
| # Skip this check for single-id jobs, or when everything fit on one page. | ||
| if expected_total is None or self._inspire_id or not had_another_page: | ||
| return | ||
|
|
||
| # One ID-only scan, then harvest anything we missed. | ||
| # Retry only if the scan collected fewer IDs than INSPIRE reported. | ||
| all_ids, scan_total = self._scan_ids(q, headers) | ||
| if scan_total is not None and len(all_ids) != scan_total: | ||
| current_app.logger.warning( | ||
| "ID scan found fewer INSPIRE records than reported. " | ||
| f"| details: found={len(all_ids)}, reported={scan_total}" | ||
| ) | ||
| retry_ids, _ = self._scan_ids(q, headers) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would the initial scan for IDs return a total hits count that's less than the number of hits it returned? Surely within one single API request the count would be consistent? Maybe I am misunderstanding this
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s not one request, _scan_ids paginates. When we first request records from a query, the first page says we have X records in this query, and that’s what scan_total is. Then when we go through each record and each page, shifting might happen while that happens and we might miss a few records while the ids are being scanned. Then scan_total and the number of ids might not be the same, so we refetch all ids with retry_ids and union the differences between all_ids and retry_ids.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh okay makes sense, thanks for the explanation.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the scan IDs will have the same problem as if we iterate record by record, I would not perform two big paginated requests since they will have the same issue... especially if we will have to deal with big harvests. |
||
| if retry_ids != all_ids: | ||
| current_app.logger.warning( | ||
| "INSPIRE ID scan shifted on retry. " | ||
| f"| details: first={len(all_ids)}, retry={len(retry_ids)}" | ||
| ) | ||
| all_ids |= retry_ids | ||
|
|
||
| # IDs we still need to fetch (in INSPIRE's list, not in what we already got). | ||
| missing_ids = all_ids - seen_ids | ||
| if not missing_ids: | ||
| return | ||
|
|
||
| current_app.logger.info( | ||
| "Re-fetching missing INSPIRE records. " | ||
| f"| details: missing={len(missing_ids)}, missing_ids={missing_ids}" | ||
| ) | ||
| for record_id in missing_ids: | ||
| data = self._get_json( | ||
| self._build_url(f"{q} AND id:{record_id}"), | ||
| headers, | ||
| ) | ||
| for inspire_record in data["hits"]["hits"]: | ||
| recovered_id = str(inspire_record["id"]) | ||
| seen_ids.add(recovered_id) | ||
| current_app.logger.debug( | ||
| f"Sending INSPIRE record #{recovered_id} to transformer." | ||
| ) | ||
| yield inspire_record | ||
|
|
||
| still_missing = all_ids - seen_ids | ||
| if still_missing: | ||
| current_app.logger.warning( | ||
| "After recovery, some INSPIRE records are still missing. " | ||
| f"| details: missing_ids={still_missing}" | ||
| ) | ||
|
|
||
| def read(self, item=None, *args, **kwargs): | ||
| """Builds a query depending on the input data.""" | ||
|
|
@@ -120,11 +211,9 @@ def read(self, item=None, *args, **kwargs): | |
| ) | ||
| query_params = {"q": f"{q} AND du >= {self._since}"} | ||
|
|
||
| base_url = "https://inspirehep.net/api/literature" | ||
| encoded_query = urlencode(query_params) | ||
| url = f"{base_url}?{encoded_query}" | ||
| url = self._build_url(query_params["q"]) | ||
|
|
||
| current_app.logger.info( | ||
| f"Resulting query: {query_params['q']}. URL for harvesting data from INSPIRE: {url}." | ||
| ) | ||
| yield from self._iter(url=url, *args, **kwargs) | ||
| yield from self._iter(url=url, q=query_params["q"], *args, **kwargs) | ||
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 realised I don't understand the order of actions that are fired in this workflow. Could we meet to discuss? I need you to walk me through what is happening :)