-
Notifications
You must be signed in to change notification settings - Fork 63
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
Support action-level concurrency in Java runtime. #130
base: master
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.
Thank you for the contribution. Have you thought about how we can unit test this change?
Not extensively. We could probably create a unit test in which we have a single container serve multiple requests simultaneously and watch for this by checking the value of a variable that gets incremented for each concurrent request or something like that. |
Hey Ben I replied on your slack thread and didn't realize you had an open pr for this will take a look |
|
||
if (Boolean.parseBoolean(System.getenv("__OW_ALLOW_CONCURRENT"))) { | ||
ThreadPoolExecutor executor = new ThreadPoolExecutor( | ||
10, // Core size. |
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'm not sure how we should optimally determine this number since every function will have a different concurrency setting. Should it err on the side of maximizing low concurrency, i.e. most functions probably won't increase their setting past 1.
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.
Agreed.
if (Boolean.parseBoolean(System.getenv("__OW_ALLOW_CONCURRENT"))) { | ||
ThreadPoolExecutor executor = new ThreadPoolExecutor( | ||
10, // Core size. | ||
25, // Max size. |
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'm not sure if we want to set the max pool size as this becomes a hard bound on the language when the openwhisk setting per function is defined elsewhere. So the user would have to know explicitly that for java they can't increase their concurrency past 25 without getting performance degradation even if they aren't cpu bounded.
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.
Yeah. This was basically an arbitrary setting on my part as I was moreso interested in getting something working. We could just as well set the max pool size to something else (or a configurable limit).
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 would be a much larger change for the system to pass in the configured action limit as a part of the payload on the init request (and not every language would need the value so the system would need special logic per language for this). I think leaving it unbounded for now is probably the easiest way to get the initial working capability and can be optimized later if needed
On top of @rabbah 's point, the action concurrency feature in openwhisk is still fairly new and experimental. It adds an additional level of capacity planning required on top of the user correctly allocating an appropriate amount of memory to the function to know how many executions an instance can support. However this will be really hard for the user to test in practice as there's no guarantee the function execution will go to the same container or really any way for the user to verify that they ran things concurrently within the same container without writing special code to recognize it in their function. Only a few that I know of are leveraging this system feature, I wonder if anyone has attempted to solve this or thought about how a user can deterministically stress test their function. Not a question for this pr, but we can move this to one of our generic forums like slack or the mailing list |
This change would enable support for action-level concurrency in the Java OpenWhisk runtime. The design is roughly modeled after the action-level concurrency support of the NodeJS environments. The Java
Proxy
checks the__OW_ALLOW_CONCURRENT
environment variable to determine whether support should be enabled or not.This does not address the action-level concurrency limit, and I imagine the values passed to the
ThreadPoolExecutor
will need to be changed for this to be used within OpenWhisk. I am not sure if the action-level concurrency configured for the action should affect the underlyingThreadPoolExecutor
object or not, though it seems like it should.At the very least, this PR serves as a starting point for evaluating whether the Java runtime can support action-level concurrency in this manner. For what it's worth, I have been using this modified runtime in personal projects for several weeks.