-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix(fhir): don’t materialise anything if the worker is disabled #7123
base: main
Are you sure you want to change the base?
Conversation
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.
Well it kinda makes sense. The downside would be that when we turn it on, we will start having a big queue but I mean, that's expected.
I think I'd be good to have yet a third pair of eyes to confirm before approving? Probably Rohan's?
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.
@passcod I just wanted to double check if this would prevent new jobs from being added to the queue?
There's a fhir.refresh_trigger
function which is used as a trigger on any of the tables that affect fhir resources. That function creates a job whenever a record in the table is updated, and I believe it's independent of this code.
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.
ugh, yeah. should we have some kind of setting that the trigger checks?
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.
Mmm yeah maybe. The integrations.fhir.enabled
setting in the config does seem like it should the primary on/off switch for fhir stuff. But reading that from the database doesn't work (and even if it was a db setting, might feel a bit awkward to check in a function).
Thoughts on maybe adding a FhirJobQueueCleaner
scheduled task or something? So we just create the jobs and separately clean them up. Does feel like extra complexity...
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.
when I originally designed settings I actually added a couple sql functions specifically to do this kinda thing (read from settings in triggers and reports). not sure they're still there?
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.
How about a check on app start that checks whether the setting is enabled or disabled, and then turns the trigger on or off to match?
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.
Yes I was thinking along the same lines after seeing chris's workaround! bonus, it would make reverting that unnecessary
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.
Are we happy to merge this PR as is and I'll make a ticket for the solution proposed by edwin to work on separate?
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.
Yes please 🙏
Changes
Not certain this is a good idea or if this should be done a different way, but currently when
fhir.worker.enabled
isfalse
, then materialisation jobs are still inserted into the queue, which means the job queue grows infinitely (which triggers alerts, and caused Samoa going down).