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
BCDA 442: Cleanup files for expired jobs #78
Conversation
9d9fd7e
to
75c01db
Compare
@@ -439,19 +458,73 @@ func removeExpired(hrThreshold int) { | |||
|
|||
id := int(j.ID) | |||
jobDir := fmt.Sprintf("%s/%d", os.Getenv("FHIR_PAYLOAD_DIR"), id) | |||
expDir = fmt.Sprintf("%s/%d", os.Getenv("FHIR_EXPIRED_DIR"), id) | |||
expDir = fmt.Sprintf("%s/%d", os.Getenv("FHIR_ARCHIVE_DIR"), id) |
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.
not a big deal but this var name should probably change 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.
I left it there because I wasn't sure if it had been used anywhere. I plan to delete it after I'm sure no clean up needs to happen (kind of like a database migration, but in reverse).
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.
makes sense
} | ||
|
||
log.WithFields(log.Fields{ | ||
"job_began": job.CreatedAt, |
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.
Also not sure about the exact logging info we want to capture here, but this seems okay to me. Might wanna get additional eyes on this part before merging.
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 ran this by Ryan yesterday. I think logging is something we should expect to evolve atm, and it's probably better to get something in the logs to work with.
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.
agreed, we can always evolve later
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.
Overall this is great, I tested locally and it's working as expected. Good stuff!!
Fixes BCDA-442
When a job is expiring due to age, its files are moved to an inaccessible location. See BCDA-441 for more about this. The work of this ticket is to cleanup that inaccessible location by removing files for jobs that have not been touched for a certain period of time.
Proposed changes:
Add a CLI command to cleanup the archive location. To run locally in docker:
docker exec -it bcda-app_api_1 bash -c 'tmp/bcda cleanup-archive --threshold 30'
--threshold is required, because I don't think it's appropriate for this argument to have a default.
Change Details
Renamed the original status associated with moving files to the archive location from 'Expired' to 'Archived'
When a job has been in 'Archived' state for
threshold
hours, remove the files from the archive and set the job status to 'Expired'Security Implications
Can only be run through CLI.
Acceptance Validation
Tests included.
Logging message
{"files_removed":"2018-11-20T07:41:19.5631881Z","job_began":"2018-11-16T16:42:00Z","job_id":1,"level":"info","msg":"Files cleaned from archive and job status set to Expired","time":"2018-11-20T07:41:19Z"}