[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665

gaol · 2024-11-20T06:28:25Z

Fixes #662

gaol · 2024-11-20T06:46:12Z

@kabir @jasondlee @fl4via may I ask you to review ? thanks :)

jasondlee · 2024-11-20T16:30:31Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+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.


Is the goal of this effort to move the sources from the external FP into the WF source tree?

No, the source code will stay in the external FP.

I added a consideration of moving it to WildFly codebase in the future when it becomes mature enough and has a higher stability level.

gaol · 2024-11-21T02:45:19Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+There is no Vertx instance set up by default, users need to do it explicitly like:
+[source,shell]
+----
+/subsystem=vertx/vertx=vertx:add()


@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 to false, so a context will be associated to the TCCL when it was created.

useDaemonThread - defaults to false, 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"); }

[1] https://github.com/wildfly/wildfly/blob/34.0.0.Final/observability/opentelemetry/src/main/java/org/wildfly/extension/opentelemetry/OpenTelemetrySubsystemRegistrar.java#L184

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

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.

Maybe we should keep the 2 options(disableTCCL, useDaemonThread) untouched by users, and open only when there are explicit demands to configure them.

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.

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

I vote for keeping those options untouched for now. We can always add later

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

kabir · 2024-11-21T11:30:55Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+
+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].


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

kabir · 2024-11-21T11:35:04Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+|===
+^| 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


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

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 · 2024-11-22T05:16:33Z

Thank you for the feedback, main updates were done:

Mention that we will consider moving the code to WildFly code base in the future
Make disableTCCL and useDaemonThread not configurable via vertx subsystem (users still can do via system property though)
Add the setting of vertx.disableDnsResolver system property in the vertx subsystem
Make the default value of fileCacheDir to ${jboss.server.temp.dir}/vertx-cache
Do not configure hostsPath, use only the alternative hostsValue for the AddressResolverOptions
Remove the function of configuring the VertxOptions from a JSON file, as it may lead to some options that are not listed above get configured which may be confusing.

darranl · 2024-12-02T16:26:53Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+promoted-by:
+---
+
+= Integrate wildfly-vertx-feature-pack to WildFly Preview


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?

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. :)

kabir

Some small suggestions but apart from that I think it looks good.
Mention/pong me when fixed since somehow I don’t get any notifications in this repository

kabir · 2024-12-04T12:31:50Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+
+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.


Should be a space before "(microprofile-reactive-messaging creates one per deployment)"

kabir · 2024-12-04T12:33:31Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+* [ ] QE
+
+=== Affected Projects or Components
+* https://github.com/wildfly/wildfly


I guess it could be good to add the feature pack repository here?

gaol · 2024-12-05T01:27:05Z

Thank you @fl4via and @kabir for the reviews and feedback, I updated accordingly. :)

bstansberry · 2024-12-06T19:06:59Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+| 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


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.

Aha, good point. But I agree on changing for the next version

bstansberry · 2024-12-06T19:07:52Z

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc

+| 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


Same comment as above re the attribute names in a 'community' stability variant:

s/hostsValue/hosts-value

github-actions bot added the stability-level/preview "Preview" stability level label Nov 20, 2024

jasondlee reviewed Nov 20, 2024

View reviewed changes

gaol commented Nov 21, 2024

View reviewed changes

microprofile/WFLY-19954_Preview_Support_vertx_feature_pack.adoc Outdated Show resolved Hide resolved

kabir reviewed Nov 21, 2024

View reviewed changes

gaol force-pushed the issues_662_vertx_feature_pack branch from 60f5ca6 to 381c0df Compare November 22, 2024 04:52

github-actions bot added stability-level/preview "Preview" stability level and removed stability-level/preview "Preview" stability level labels Nov 22, 2024

jasondlee approved these changes Nov 26, 2024

View reviewed changes

gaol mentioned this pull request Nov 27, 2024

[Issue-40] Adds missing attributes wildfly-extras/wildfly-vertx-feature-pack#41

Merged

darranl reviewed Dec 2, 2024

View reviewed changes

gaol force-pushed the issues_662_vertx_feature_pack branch from 381c0df to 4a43c58 Compare December 3, 2024 13:25

github-actions bot added stability-level/preview "Preview" stability level and removed stability-level/preview "Preview" stability level labels Dec 3, 2024

gaol changed the title ~~[WFLY-19954] Add the wildfly-vertx-feature-pack to WildFly Preview~~ [WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack Dec 3, 2024

fl4via approved these changes Dec 4, 2024

View reviewed changes

kabir requested changes Dec 4, 2024

View reviewed changes

[WFLY-19954] Add the wildfly-vertx-feature-pack to WildFly Preview

4113ccc

gaol force-pushed the issues_662_vertx_feature_pack branch from 4a43c58 to 4113ccc Compare December 5, 2024 01:25

github-actions bot added stability-level/preview "Preview" stability level and removed stability-level/preview "Preview" stability level labels Dec 5, 2024

kabir approved these changes Dec 5, 2024

View reviewed changes

bstansberry reviewed Dec 6, 2024

View reviewed changes

gaol mentioned this pull request Dec 8, 2024

Rename all attributes to follow the WildFly style wildfly-extras/wildfly-vertx-feature-pack#48

Open

darranl merged commit f093382 into wildfly:main Dec 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665

[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665

gaol commented Nov 20, 2024

gaol commented Nov 20, 2024

jasondlee Nov 20, 2024

gaol Nov 21, 2024

gaol Nov 22, 2024

gaol Nov 21, 2024

kabir Nov 21, 2024

jasondlee Nov 21, 2024

gaol Nov 22, 2024

gaol Nov 22, 2024

jasondlee Nov 26, 2024

fl4via Dec 4, 2024

kabir Nov 21, 2024

kabir Nov 21, 2024

gaol Nov 21, 2024

gaol commented Nov 22, 2024

darranl Dec 2, 2024

gaol Dec 3, 2024

kabir left a comment

kabir Dec 4, 2024

kabir Dec 4, 2024

gaol commented Dec 5, 2024

bstansberry Dec 6, 2024

kabir Dec 6, 2024

bstansberry Dec 6, 2024


		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].

[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665

[WFLY-19954] Add the vertx extension/subsystem from wildfly-vertx-feature-pack to WildFly Preview Feature Pack #665

Conversation

gaol commented Nov 20, 2024

gaol commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaol commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kabir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaol commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment