-
Notifications
You must be signed in to change notification settings - Fork 294
[RemoteRuntime] Add option to enable async mode when using `nuclio' runtime with http trigger #9141
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
davesh0812
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.
Nice
theSaarco
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.
A small fix is needed.
mlrun/runtimes/nuclio/function.py
Outdated
| "Async spec is only supported on Nuclio 1.15.3 and higher" | ||
| ) | ||
| if async_spec.enabled: | ||
| trigger._struct["mode"] = "async" if async_spec.enabled else "sync" |
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.
You get here since async_spec.enabled, so there's no way this will be sync.
liranbg
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.
very well, minor comment
tests/system/runtimes/test_nuclio.py
Outdated
| ) | ||
| function.spec.function_handler = "main:async_handler" | ||
|
|
||
| function.with_http(async_spec=AsyncSpec(enabled=True, max_connections=900)) |
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.
isnt 100 enough for this test?
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.
100 provide latency of 10 seconds passed with 200 with 5.5 seconds
theSaarco
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
📝 Description
This includes adding a async configuration while using
RemoteRuntime.with_http.initialization of default http trigger in async mode if- disabled till NUC-704 is donespec.graph.engineis "async"🛠️ Changes Made
AsyncSpecdataclass and adding it as input param forwith_httpdefault http with async engine(disabled) or by usingfunction.with_http(AsyncMode(enabled=True)✅ Checklist
🧪 Testing
Adding system test
TestNuclioRuntime:test_async_http_mode🔗 References
🚨 Breaking Changes?
🔍️ Additional Notes