-
Notifications
You must be signed in to change notification settings - Fork 457
Multiple workers #4210
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Multiple workers #4210
Conversation
18a3b41
to
3997b85
Compare
594f54e
to
d3fe2b5
Compare
b96570b
to
ef9cf5f
Compare
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcher.cs
Outdated
Show resolved
Hide resolved
ef9cf5f
to
cb73853
Compare
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcher.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Outdated
Show resolved
Hide resolved
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.
Another round of some minor comments...
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Show resolved
Hide resolved
7e95168
to
6b2f6a6
Compare
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.
This is a partial review. Sending an initial round of feedback as I didn't want to block. Will continue, but wanted to get this in front of you.
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcher.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherLoadBalancer.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcherState.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script/Rpc/FunctionRegistration/IFunctionDispatcherLoadBalancer.cs
Show resolved
Hide resolved
@maiqbal11 - For testing python functions |
Adding notes from my testing. I'm using the app that was blocking on calls between functions in the same app from the following issue: Azure/azure-functions-python-worker#236. There is a queue trigger that triggers an http invocation.
There are still concerns that the blocking behavior will manifest if invocations land on the same process. For example, if the user configures two processes and the calls come in the following sequence: (a) queue message 1 -> process 1 (a) and (c) above would block each other since (a) is waiting for (c) to finish with the same thread in Python and vice versa. The interim mitigation here would be to use |
Thanks @maiqbal11 for testing. I found the root cause. Invocations on any trigger need to wait for the Host to be ready. Updated WorkerLanguageInvoker to wait for scripthost to be ready before accepting invocations. |
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.
Sending a round of feedback... will follow up on some things in person
@@ -164,7 +165,15 @@ private async Task StartHostAsync(CancellationToken cancellationToken, int attem | |||
if (!startupMode.HasFlag(JobHostStartupMode.HandlingError)) | |||
{ | |||
LastError = null; | |||
|
|||
var functionDispatcher = _host.Services.GetService<IFunctionDispatcher>(); |
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.
It's not immediately clear what the purpose of this code is. Adding some comments will help. A clearer log message would also be helpful.
It also feels like this should be elsewhere, as it shouldn't be the responsibility of the Script Host service
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.
Will add comments. Need to wait for at least one language worker process to be initialized before setting ScriptHostState to running. Let me know if there is a better place add this change.
src/WebJobs.Script/Rpc/FunctionRegistration/FunctionDispatcher.cs
Outdated
Show resolved
Hide resolved
@pragnagopa, did a second round with the new code. Didn't see the issue where a queue message being present before host startup was causing an invocation before the language worker was up. However, even though I believe the message got handled correctly since I no longer see it on the queue, there were no longs to indicate this. The processing seemed to happen silently without any success or failure logs. |
// Need to wait for atleast one language worker process to be initialized before setting ScriptHostState to running | ||
// for non dotnet functions | ||
if (_functionDispatcher != null && _functionDispatcher.State == FunctionDispatcherState.Initializing) | ||
{ |
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.
@fabiocav - Moved the logic that checks for function dispatcher start to WorkerLanguageInvoker. Can you please take a final look?
1c4cc8a
to
13a6a7d
Compare
6252edc
to
829cc9c
Compare
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.
Haven't looked at the last round in detail, but there should be no blockers based on the last iteration. We can address any additional comments later.
FUNCTIONS_WORKER_PROCESS_COUNT
FUNCTIONS_WORKER_PROCESS_COUNT
is set to greater than 1, language worker is created every 10 seconds. This is to avoid impact on cold startFixes #4195 Fixes #4194 Fixes #4193 Fixes #4161 Fixes #3939 Fixes #4026 Fixes #4326 Fixes #4392
Added by mhoeger: Fixes #4328