-
Notifications
You must be signed in to change notification settings - Fork 107
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
CUMULUS-2688: Update bulk operations to fetch granules by unique columns #3000
CUMULUS-2688: Update bulk operations to fetch granules by unique columns #3000
Conversation
…/CUMULUS-2688-bulk-operation
@@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). | |||
|
|||
## Unreleased Phase 3 | |||
|
|||
### Breaking Changes |
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.
Wondering if there should be migration instructions for this... 🤔.
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.
Definitely requires an update to https://github.com/nasa/cumulus-api/
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.
Yeah, good call. I'll update that repo once all three tickets for 2688 are in as they all have changes that'll need to be noted. It looks like we might need a new rds-phase-3
branch there that gets released at the same time as the main cumulus phase 3 release too 🤔
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.
const { reingestGranule, applyWorkflow } = require('../lib/ingest'); | ||
|
||
const log = new Logger({ sender: '@cumulus/bulk-operation' }); | ||
|
||
async function applyWorkflowToGranules({ | ||
granuleIds, | ||
granules, |
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.
These aren't really granules. They're { collectionId, granuleId}
. If this is confusing I can rename.
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.
Yeah, I think it should be something better across the stack, esp at the endpoint level if we're renaming there anyway. I just can't decide on a good name. 🤔
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.
Yeah that's where I'm getting stuck. It's really granuleUniqueIdsButNotCumulusIds
😄. Though the API doesn't return cumulus_id
at all so maybe it would only be slightly confusing on the dev side.
We could do something like granuleAndCollectionIds
but that could also be ambiguous because "ID" can mean a couple things in our schema. It highlights the problem of not exposing cumulus_id
to these endpoints/users.
The more I think about it the more I'm leaning towards keeping granules
. My reasoning (subject to change) is that you could pass an entire API Granule object and it would work. It's just that we really only need granuleId and collectionId.
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 can't find a convincing reason to disagree.
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.
All the changes look fine, but I'd really like to avoid overloading granules
even more in this way. Let's discuss regarding another term when you're available.
Summary:
Addresses CUMULUS-2688
This is PR 2/3 for the above ticket. This PR address the changes to bulk operations. When a bulk operation operates on granules it now needs to accept and fetch granules by
granuleId
+collectionId
, not justgranuleId
. This is because the unique identifiers changed in the Postgres switchover.Changes
BULK_GRANULE
,BULK_GRANULE_DELETE
, andBULK_GRANULE_REINGEST
operations in bulk-operations to support collectionIdPR Checklist