-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Enable support for project loom for Jetty's thread pool #7457
Conversation
@dropwizard/committers This implementation should be enough to enable virtual threads. For basic testing we could ship with this implementation and add support for virtual thread executors in the Blocked by dropwizard/metrics#3531 |
dropwizard-core/src/main/java/io/dropwizard/core/server/AbstractServerFactory.java
Outdated
Show resolved
Hide resolved
18dc003
to
6ebebb1
Compare
Do any of the other thread based properties need to be handled/documented differently when virtual is enabled?
|
116392f
to
27f42f6
Compare
27f42f6
to
99c0d52
Compare
Thanks for working on supporting project loom, is there a plan to backport this change to release 3.x? |
Hi @cty123. As mentioned in the release notes, Dropwizard 3.x and 4.x should be kept in sync. Since the |
@dropwizard/committers Anyone available for this PR? |
minThreads, maxThreads and idleTimeout are set with the constructor of the main acceptorThreads and selectorThreads obtain threads from the same thread pool and therefore will also use virtual threads. For the admin connector the |
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 general, LGTM.
I do wonder if we shouldn't fail more obviously in the case that the user configures enableVirtualThreads: true
and it can't work (either isn't supported or the reflection code fails). It seems like Dropwizard's typical approach is to fail rather than quietly ignore a config setting (beyond a warn log statement).
@pstackle Thanks for the fast review! I wanted to introduce the logs only for an easier migration path. But I agree with you, that users should be notified more than that, if the application cannot do what's requested. I've revised the PR and the code now throws exceptions. I additionally added support for virtual threads on the admin connectors. Then the behavior of the connectors is more similar. But for the separate thread pool there is a separate configuration option to enable it. Do you think we should provide any other documentation than the configuration options? I'm not sure it's a topic this big since most users will most likely not directly migrate to Java 21 and make use of virtual threads. The interested users probably have already seen the issue and this pr 😉 If you think so, I'll leave a short note in the core docs. |
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.
Found one nitpick in an exception message, but otherwise, I'm happy with this.
dropwizard-core/src/main/java/io/dropwizard/core/server/AbstractServerFactory.java
Outdated
Show resolved
Hide resolved
I think this feature is likely one that is worth calling out specifically in the release notes, because this is one of the main features in Java 21. I can't really think of a place in the other docs that it would make sense to reference this. Especially because we'd have to caveat it with that you must be running Java 21 or greater to even enable it. |
Sounds good 👍 |
Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com>
ef3295e
to
8da2eaa
Compare
* Enable support for project loom for Jetty's thread pool * Add support for virtual threads on the admin connectors Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com> --------- Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com> Refs dropwizard#7457 (cherry picked from commit 5fbcb6d)
* Enable support for project loom for Jetty's thread pool * Add support for virtual threads on the admin connectors Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com> --------- Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com> Refs #7457 (cherry picked from commit 5fbcb6d)
This PR provides a new configuration option to enable virtual threads for Jetty's thread pool.
If the current runtime doesn't support virtual threads, platform threads are used instead.