-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Transition VirtualThreadsConfig
to use @ConfigMapping
#45749
Conversation
gastaldi
commented
Jan 21, 2025
- Remove legacy configuration compiler arguments from Maven POM files to streamline build configuration.
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
VirtualThreadsConfig
to use @ConfigMapping
.VirtualThreadsConfig
to use @ConfigMapping
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...s/virtual-threads/runtime/src/main/java/io/quarkus/virtual/threads/VirtualThreadsConfig.java
Show resolved
Hide resolved
...virtual-threads/runtime/src/main/java/io/quarkus/virtual/threads/VirtualThreadsRecorder.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview c5ae9ab has been successfully built and deployed to https://quarkus-pr-main-45749-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
@manofthepeace @cescoffier I've changed the config to honor the default values, tests were failing without it |
This comment has been minimized.
This comment has been minimized.
3363668
to
882212e
Compare
- Remove legacy configuration compiler arguments from Maven POM files to streamline build configuration.
Status for workflow
|
Status for workflow
|
Related to #45446 |
|
||
/** | ||
* Virtual thread name prefix. If left blank virtual threads will be unnamed. | ||
* Virtual thread name prefix. The name of the virtual thread will be the prefix followed by a unique number. | ||
* The default value is "quarkus-virtual-thread-". |
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 why you added that? The generated doc takes care of it.
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.
Ah forgot about that, I'll fix in a separate PR
@@ -20,7 +20,7 @@ public class VirtualThreadsRecorder { | |||
|
|||
private static final Logger logger = Logger.getLogger("io.quarkus.virtual-threads"); | |||
|
|||
static VirtualThreadsConfig config = new VirtualThreadsConfig(); | |||
static volatile VirtualThreadsConfig config; | |||
|
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.
Is there anything specific here that prevents us from using injection in build steps?
In the test below we could pass the config as a parameter?
(sorry on my phone in a waiting room…)
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 kept the original code because I didn't want to break anything, but maybe I can improve that
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 fine with me, I was just surprised. We had quite a lot of bad surprises with this pattern.
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 doesn't look like it's avoidable. It is necessary to create a new executor (io.quarkus.virtual.threads.VirtualThreadsRecorder#createExecutor
) and I don't see a better way to do that without looking up the object again