-
Notifications
You must be signed in to change notification settings - Fork 518
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
Disable JobHosting workers for disabled jobs #4752
base: main
Are you sure you want to change the base?
Disable JobHosting workers for disabled jobs #4752
Conversation
/// <summary> | ||
/// Removes queues based on the enabled status of the operations. | ||
/// </summary> | ||
public void RemoveDisabledQueues() |
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.
Maybe a silly question, why do we add them then remove instead of skipping to add?
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 thought the same thing initially. 😆 The main reasoning is this approach allows default configuration applied in appsettings.json
which is parsed to the configuration objects. I think this is the correct approach vs applying overrides via environment variables/appsettings overrides in the hosting environment which have to be maintained separately.
So once we parse the defaults into the configuration class, why have the removal logic here? I'm thinking keeping it in this class encapsulates the logic here vs polluting the registration classes with operation specific logic. It just exposes a hook to be called.
I was on the fence about implementation on this - happy to change the approach if there are thought a different direction.
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.
@brendankowitz - I could use a custom ConfigurationProvider
instead like below. Thoughts?? I think this would intercept the configuration of the class.
Line 22 in cb53a35
public class DictionaryExpansionConfigurationProvider : ConfigurationProvider |
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.
In thinking about it, perhaps this is calling more to consolidate these settings into one place. Right now we enable/disable in one place, configure job settings in another and remove as a post step. It might make more sense to move all these into a single config which would make this more discoverable / maintainable
Description
Related issues
AB#134785
Testing
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)