[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665
Conversation
|
@kabir @jasondlee @fl4via may I ask you to review ? thanks :) |
| N/A | ||
|
|
||
| == Implementation Plan | ||
| The {wildfly_vertx_feature_pack_with_link} repository is hosted under wildfly-extras github organization. It contains the source codes for all subsystem functionalities, unit tests and integration tests, including the tests on the vertx subsystem management model operations. |
There was a problem hiding this comment.
Is the goal of this effort to move the sources from the external FP into the WF source tree?
There was a problem hiding this comment.
No, the source code will stay in the external FP.
There was a problem hiding this comment.
I added a consideration of moving it to WildFly codebase in the future when it becomes mature enough and has a higher stability level.
| There is no Vertx instance set up by default, users need to do it explicitly like: | ||
| [source,shell] | ||
| ---- | ||
| /subsystem=vertx/vertx=vertx:add() |
There was a problem hiding this comment.
@jasondlee @kabir Do you see any options that we need to stay using the default ones ?
I am not sure if the following options should be updated by users or not:
disableTCCL- defaults tofalse, so a context will be associated to the TCCL when it was created.useDaemonThread- defaults tofalse, so all Vertx threads are not daemons.
Also do you see some system properties that we need to set before constructing the Vertx instance in microprofile-opentelemetry and microprofile-reactive-messaging subsystem ?
One example I see coming from opentelemetry subsystem[1]:
// We need to disable vertx's DNS resolver as it causes failures under k8s
if (System.getProperty(VERTX_DISABLE_DNS_RESOLVER) == null) {
System.setProperty(VERTX_DISABLE_DNS_RESOLVER, "true");
}
There was a problem hiding this comment.
At least for reactive messaging I'm not setting any properties in WildFly. I don't know if any are set internally in SmallRye RM. I guess not since when running in Quarkus they use the Vert.X instance provided by Quarkus
There was a problem hiding this comment.
I am not sure if the following options should be updated by users or not:
Those sound like properties that I'm not sure we'd want users managing inside an application server, but I could be wrong.
One example I see coming from opentelemetry subsystem[1]:
That's kind of an ugly hack that we had to put in because vertx did not (and may still not. I haven't looked) provide an API to set. I don't know if setting that property in opentelemetry will affect the vertx instance from the FP. I'd be surprised if it's set in time, given the module system lifecycle, etc., so we might either need to make that a configurable option, or just add that to the FP.
There was a problem hiding this comment.
Maybe we should keep the 2 options(disableTCCL, useDaemonThread) untouched by users, and open only when there are explicit demands to configure them.
There was a problem hiding this comment.
because vertx did not (and may still not. I haven't looked) provide an API to set ...
so we might either need to make that a configurable option, or just add that to the FP.
I don't see any API either to set, so probably be good to add that to the FP as well before a Vertx instance is constructed so that it takes effect.
There was a problem hiding this comment.
For those curious and unfamiliar with this issue, the code in question is here: https://github.com/eclipse-vertx/vert.x/blob/4.x/src/main/java/io/vertx/core/spi/resolver/ResolverProvider.java#L35-L50
There was a problem hiding this comment.
I vote for keeping those options untouched for now. We can always add later
microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc
Outdated
Show resolved
Hide resolved
|
|
||
| The demand comes from https://github.com/smallrye/smallrye-reactive-messaging/discussions/2725[a discussion in smallrye-reactive-messaging] to ask the capability to configure the vertx options used in the microprofile-reactive-messaging subsystem, and there is a {wildfly_vertx_feature_pack_with_link} ready to fulfill, integrate it to WildFly should satisfy the requirement. | ||
|
|
||
| The goal of the {wildfly_vertx_feature_pack_with_link} is to be the central place to provide the Vertx instance within WildFly server for other subsystems which may need it, example subsystems are https://github.com/wildfly/wildfly/tree/main/microprofile/telemetry-smallrye[microprofile-telemetry] and https://github.com/wildfly/wildfly/tree/main/microprofile/reactive-messaging-smallrye[microprofile-reactive-messaging]. |
There was a problem hiding this comment.
Might be worth pointing out that these currently create their own vertx instances (and I think it is actually worse, since at least RM will create one per deployment). Advice from the VertX team is to have only one shared instance
| |=== | ||
| ^| Field Name ^| Data Type ^| Source Class ^| Resource Attribute Name ^| Note | ||
|
|
||
| | hostsPath | String | {address_resolver_options_with_link} | hosts-path | the path to the alternate hosts configuration file |
There was a problem hiding this comment.
What are the contents of this file?
For domain mode, it might be problematic to have things in external files. It might be better to make this part of the management model, and have the subsystem create some kind of temp file which then is passed in to vertx
There was a problem hiding this comment.
The content is something like what is inside /etc/hosts in Linux. The hostsValue is the alternative which contains the content in String. So maybe we can just support hostsValue.
gaol
force-pushed
the
issues_662_vertx_feature_pack
branch
from
60f5ca6 to
381c0df
Compare
github-actions
bot
added
stability-level/preview
|
Thank you for the feedback, main updates were done:
|
| promoted-by: | ||
| --- | ||
|
|
||
| = Integrate wildfly-vertx-feature-pack to WildFly Preview |
There was a problem hiding this comment.
Just a small comment about terminology here, WildFly Preview is itself a feature pack - is this proposal more about adding the Vert.x subsystem / extension to the WildFly Preview Feature Pack?
There was a problem hiding this comment.
Yes, it is. I thought WildFly Preview is a different distribution, so Adding the vertx subsystem / extension to the WildFly Preview Feature Pack would be more accurate, will update it. :)
gaol
force-pushed
the
issues_662_vertx_feature_pack
branch
from
381c0df to
4a43c58
Compare
github-actions
bot
added
stability-level/preview
gaol
changed the title
|
|
||
| The demand comes from https://github.com/smallrye/smallrye-reactive-messaging/discussions/2725[a discussion in smallrye-reactive-messaging] to ask the capability to configure the vertx options used in the microprofile-reactive-messaging subsystem, and there is a {wildfly_vertx_feature_pack_with_link} ready to fulfill, integrate it to WildFly should satisfy the requirement. | ||
|
|
||
| The goal of the {wildfly_vertx_feature_pack_with_link} is to be the central place to provide the Vertx instance within WildFly server for other subsystems which may need it, example subsystems are https://github.com/wildfly/wildfly/tree/main/microprofile/telemetry-smallrye[microprofile-telemetry] and https://github.com/wildfly/wildfly/tree/main/microprofile/reactive-messaging-smallrye[microprofile-reactive-messaging]. Currently, they create their own vertx instances(microprofile-reactive-messaging creates one per deployment). Advice from the VertX team is to have only one shared instance for everything. |
There was a problem hiding this comment.
Should be a space before "(microprofile-reactive-messaging creates one per deployment)"
| * [ ] QE | ||
|
|
||
| === Affected Projects or Components | ||
| * https://github.com/wildfly/wildfly |
There was a problem hiding this comment.
I guess it could be good to add the feature pack repository here?
gaol
force-pushed
the
issues_662_vertx_feature_pack
branch
from
4a43c58 to
4113ccc
Compare
github-actions
bot
added
stability-level/preview
| | classPathResolvingEnabled | boolean | {filesystem_options_with_link} | classpath-resolving-enabled | whether classpath resolving is enabled | ||
| | fileCachingEnabled | boolean | {filesystem_options_with_link} | file-cache-enabled | whether file caching is enabled for class path resolving | ||
| | fileCacheDir | String | {filesystem_options_with_link} | file-cache-dir | file cache directory, defaults to `${jboss.server.temp.dir}/vertx-cache` | ||
| | addressResolverOptions | AddressResolverOptions | {vertx_options_with_link} | address-resolver-option | reference to the AddressResolverOptions name |
There was a problem hiding this comment.
I don't think should block integration at 'preview' stability, but for 'community' we should consider changing all these attributes to follow the WildFly style.
s/eventLoopSize/event-loop-size
etc
AFAIK there are no deviations from this style in subsystem's that ship in WildFly.
There was a problem hiding this comment.
Aha, good point. But I agree on changing for the next version
| | searchDomains | List<String> | {address_resolver_options_with_link} | search-domains | the list of search domains | ||
| | ndots | int | {address_resolver_options_with_link} | n-dots | the ndots value | ||
| | rotateServers | boolean | {address_resolver_options_with_link} | rotate-servers | if the dns server selection uses round-robin | ||
| | roundRobinInetAddress | boolean | {address_resolver_options_with_link} | round-robin-inet-address | if the inet address selection uses round-robin |
There was a problem hiding this comment.
Same comment as above re the attribute names in a 'community' stability variant:
s/hostsValue/hosts-value
Fixes #662