feat: Resource Archive Support #64
Conversation
…onal unix epoch TTL
…cached yarn dependency causing rukus
…can be found by yarn in node_modules/.bin and not requiring a global tsc
…ing git urls... Rolling back so I can test with npm
src/persistence.ts
Outdated
| @@ -9,6 +9,7 @@ export interface CreateResourceRequest { | |||
| resourceType: string; | |||
| resource: any; | |||
| id?: string; | |||
| ttl?: number; // used for archiving | |||
There was a problem hiding this comment.
Maybe we should add in the comment that this value should be timeFromEpoch value in ms. Also do we need to add this field for Update and Patch requests?
There was a problem hiding this comment.
Thoughts on changing this to ttlInSeconds to make it more explicit, what the value should be? By default DDB requires this value to be in seconds, but in JS Date.now() returns the time in ms.
There was a problem hiding this comment.
Yeah, I can add a more detailed comment and documentation here. Sorry, it's actually the timestamp in Unix epoch timestamp in seconds, which matches the dynamoDB ttl definition. I will update the comment and documentation to match that of the dynamoDB documentation of ttl here.
I started down the path of updating the ttl on put and patch but didn't want to take a risk the risk of breaking the transaction logic. I figured an absolute expiration as opposed to a sliding expiration would be cleaner right now. It also avoids the non-deterministic scenario where a dynamoDB item has technically expired but the background scanner process hasn't yet picked up the item and started processing yet. Which that time frame looks to be documented at up to 48 hours after a ttl expiration. I'm not super passionate on this though so if you think a sliding expiration makes more sense, I can spend the time to add the logic for put and patch.
There was a problem hiding this comment.
In general we want to avoid making many small separate changes to the interface, which is why I think it's better to add those fields to PATCH and PUT on the interface now. Since those fields are optional, we leave it to the implementer of the other packages whether they want to take advantage of ttlInSeconds in their PATCH or PUT operations.
Technically, even if we support CREATE only, wouldn't it still be possible that an item that is supposed to have expire isn't deleted till 48 hours later?
We can set a rule in the persistence package not to accept CREATE, PATCH, or PUT requests with ttlInSeconds that are in the past. We will also have an SLA that those items are not guaranteed to be deleted immediately when they expire, but will be deleted within 48 hours.
There was a problem hiding this comment.
Yeah, this makes a lot of sense on both accounts. I will update the variable name to ttlInSeconds and add to the PATCH and PUT interfaces now. I'll also spend some time to go back and try and implement the PATCH and PUT flows to support a sliding expiration. The configuration upstream could then allow someone to set either an absolute or sliding expiration per resource.
Thanks for your help here and quick turnaround.
…ode comment and added to update and bundle paths
|
@nguyen102 Just an FYI, I have updated persistence, routing and deployment for the interface changes, PUT and PATCH support and bundle support. However, the implementation has a snag where any updates are assigning the new ttlInSeconds to only the last version of the resource instance. I'm going to try and see if I can find a way to get all versions of resource instances to have the ttlInSeconds updated when the resource instance is updated so we do not end up with a state where some versions are archived and others are not. |
|
@Zambonilli thanks for your help on this. Feel free to open up those PRs when you're ready for review. |
Issue #, if available:
issue #290
awslabs/fhir-works-on-aws-deployment#290
Description of changes:
Add nullable unix epoch ttl field to resources so persistence implementations can auto-archive. Apologies on the pushes moving typescript around. yarn 1.x has an issue when using git dependency URLs the devDependencies are not resolved. npm works fine though.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.