-
Notifications
You must be signed in to change notification settings - Fork 23
Add capabilities update strategy to environment reload response #95
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
Conversation
ejizba
left a comment
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.
Cool! Just fyi, if/when the Node.js worker uses this we will probably use 'replace' just so that we can handle any merging ourselves
We will probably do the same for the PowerShell language worker. |
Francisco-Gamino
left a comment
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.
LGTM
|
@satvu @brettsam Based on the known cases, do you know if removing capabilities on the worker side after specialization is intentional, and not just a bug on the worker side? I can imagine such cases in theory, but I'm wondering if we have a legitimate use case for that. Otherwise, perhaps 'replace' is all we ever need? |
@AnatoliB I don't believe we have a use case for removing capabilities, but to answer your last question, "merge" is an option and our current choice for default since it best describes the host's behavior today. We do currently update capability values - it's the re-application/parsing of capabilities following an environment reload that is the bug. Moving to just "replace" will require more validation as it is different from the current behavior, and we do not want to accidentally break anyone by completely switching to a new strategy (scenario 1 in description). Given the popularity of "replace" though, the "merge" option may end up being unused in the future. |
|
Yeah, "merge" may be unsupported at some point. I'd assume most workers would want to figure it all out on their own and just send a completely new list with "replace". But we saw the python worker today sends us an empty payload on Environment Reload. So instead of special-casing this, we decided to make "merge" a first-class option and make it the default. |
Even though updated capabilities are sent in an environment reload response, they are not always applied correctly (Related host PR: Azure/azure-functions-host#9367). Furthermore, there may be cases where we need different strategies for applying capabilities. Some examples from @brettsam:
This PR adds a property to the environment reload response message indicating to the host which strategy to use when receiving updated capabilities from a worker.