fix(harvester): recover records skipped by live pagination - #886
fix(harvester): recover records skipped by live pagination#886TahaKhan998 wants to merge 1 commit into
Conversation
|
|
||
| # Pagination can skip records when INSPIRE results shift mid-harvest. | ||
| # A total mismatch can also mean records entered/left the live query. | ||
| if not q or expected_total is None or len(seen_ids) == expected_total: |
There was a problem hiding this comment.
can a record be added to the set and cause a shift in another direction? what if one record is added and another removed?
There was a problem hiding this comment.
yeah good point, if one record drops out and another joins the total can stay the same and we'd never hit recovery with the count check alone.
i changed it to always do the id scan at the end and recover whatever's in all_ids - seen_ids, so we're comparing the actual sets not just the count.
| # Re-scan with IDs only, then harvest the missing records by id. | ||
| all_ids = set() | ||
| ids_url = self._build_url(q, fields="id") | ||
| while ids_url: |
There was a problem hiding this comment.
what if something shifts in the rescan?
There was a problem hiding this comment.
Yes the rescan can shift too since it's still paginated.
i changed so now we run the id scan twice with size=1000, and if the two passes don't match we union them and log a warning. anything still missing gets fetched individually by id instead of re-paginating the full harvest. i also added a test for this case(test_reader_recovers_when_count_matches).
i also have another approach in mind where we collect all ids upfront at the start and then harvest each record by id instead of paginating through search at all. that'd be more robust but more expensive api-wise, so i kept this for now since it fits the current flow. happy to switch if you'd prefer that.
3c6abbe to
8ee741a
Compare
8ee741a to
2f637d7
Compare
palkerecsenyi
left a comment
palkerecsenyi
left a comment
There was a problem hiding this comment.
Looks good, it's a nice and efficient way of doing it! I just have one question
| "ID scan found fewer INSPIRE records than reported. " | ||
| f"| details: found={len(all_ids)}, reported={scan_total}" | ||
| ) | ||
| retry_ids, _ = self._scan_ids(q, headers) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh okay makes sense, thanks for the explanation.
3 participants
Closes #437
INSPIRE search results can change while we're paginating. If a record gets updated mid-harvest, it can jump to a page we already passed, so we never pick it up even though hits.total said it was there.
What we do now: during the normal harvest we keep the IDs we actually saw, plus the total from the first page. If those don't match at the end, we re-query the same search with fields=id to get the full ID list, figure out what's missing (all_ids - seen_ids), and fetch only those records with q=... AND id:... like we already do for single-id harvests. If everything matched on the first pass, we don't do any of that extra work.