CUMULUS-3806 update Orca Recon Report to use postgres as source of truth #3794
Conversation
packages/api/tests/lambdas/test-orca-backup-reconciliation-report.js
Outdated
Show resolved
Hide resolved
| @@ -612,114 +623,6 @@ describe('When there are granule differences and granule reconciliation is run', | |||
| }); | |||
| }); | |||
|
|
|||
| // TODO: the internal report functionality will be removed after collections/granules is changed to no longer use ES | |||
| xdescribe('Create an Internal Reconciliation Report to monitor internal discrepancies', () => { | |||
There was a problem hiding this comment.
This test is removed as internal reports are going away.
|
|
||
| const pgGranulesIterator = new QuerySearchClient( | ||
| granulesSearchQuery, | ||
| 100 // arbitrary limit on how items are fetched at once |
There was a problem hiding this comment.
Reviewers: looking for opinions on value/approach here. Given we're explicitly bounding this on performance testing per team decision/given existing limitations in the tool, I'm not sure if that veers into YAGNI given this might need refactored. We're using this value for the other recon reports, however....
Jkovarik
changed the title
| const { | ||
| granules: granulesTable, | ||
| collections: collectionsTable, | ||
| providers: providersTable, | ||
| } = TableNames; | ||
| const temporalColumn = temporalBoundByCreatedAt ? 'created_at' : 'updated_at'; |
There was a problem hiding this comment.
We don't have index on granules.created_at. Do we need to create one?
There was a problem hiding this comment.
@jennyhliu that's a complicated question. We should discuss with the PO re: perf requirements.
There was a problem hiding this comment.
Why do we update this if 'internal' reconciliation report is no longer needed?
There was a problem hiding this comment.
Great question - this should just be removed along with it's units/etc. I'd been originally planning on doing that in a separate PR, but at this point it can just be included here.
There was a problem hiding this comment.
@jennyhliu - after doing work on downstream parts of this ticket, I'm going to punt that to PR#3 in this ticket.
| const nextOrcaId = `${nextOrcaItem.id}:${nextOrcaItem.collectionId}`; | ||
| if (nextCumulusId < nextOrcaId) { | ||
| // Found an item that is only in Cumulus and not in ORCA. | ||
| addGranuleToReport({ | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await addGranuleToReport({ |
There was a problem hiding this comment.
so addGranuleToReport is likely called for every granule to retrieve files?
There was a problem hiding this comment.
@jennyhliu correct - this will increase the number of database calls, but by making them per-granule given how the rec reports are currently setup this should effectively disperse that load.
We absolutely need to improve the overall setup for these tests, that work should neatly fall under the 'improve rec reports' epic.
|
@jennyhliu I believe I've addressed your comments, thanks for the review! 🙇🏻 feature has also been merged in. |
Summary: Summary of changes
Addresses Internal/Orca reconciliation report components from CUMULUS-3806
Changes
@cumulus/db/lib/granule.getGranulesByApiPropertiesQuerytobe parameterized and include a modifier on
temporalBoundByCreatedAtand updated API to throw an error if report is requested
postgres via
getGranulesByApiPropertiesQueryPR Checklist