Skip to content
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

[WFCORE-6873] Remoting: use subsystem type endpoint for outbound comm… #6055

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tadamski
Copy link
Contributor

@tadamski tadamski commented Jul 1, 2024

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 1, 2024
@wildfly-ci

This comment was marked as outdated.

@yersan yersan requested review from ropalka and fl4via July 2, 2024 10:26
@yersan
Copy link
Collaborator

yersan commented Jul 2, 2024

Hello @fl4via / @ropalka Could anyone of you review this? I am not familiar with the ContextManager and what this setGlobalDefault implies, thanks

@wildfly-ci

This comment was marked as outdated.

@baranowb
Copy link
Contributor

baranowb commented Jul 4, 2024

Generally would be good to point why this change will affect outbound comm.(given comment above alone) @tadamski ^^

@bstansberry
Copy link
Contributor

@tadamski @fl4via How is this tested?

@rachmatowicz
Copy link
Contributor

rachmatowicz commented Jul 9, 2024

Not so sure about this change. The Contextual Endpoint is used by all client code (Naming client, EJB client, Transaction client) to initiate connections from the client to a server. The wildfly-config.xml file is used to configure this contextual as well as the others that are available to the client side code (EJBClientContext, Discovery, AuthenticationContext, XnioWorker, ...). Generally, these various Contextuals are initialized statically by parsing the wildfly-config.xml file and setting a global default in the context manager, whether the client code is running on a standalone client or within a deployment on a server.

This change will cause the Contextual Endpoint to point to the Endpoint defined in the Remoting subsystem, and not the usual Contextual Endpoint initialized with wildfly-config.xml. The possible problems I see are:
(1) now to configure the client-side connection properties for any Wildfly client code in a deployment, we cannot use the wildfly-client.xml file and need to configure the client-side properties in the Remoting subsystem Endpoint configuration, which will have to be the same for all clients
(2) the XnioWorker instance will still be configured from wildfly-config.xml and not from within the Remoting subsystem, which is a bit confusing as the Endpoint and XnioWorker are so intimately related

It seems to me that configuring outbound connection establishment using wildfly-config.xml and configuring inbound connection establishment using the Remoting subsystem Endpoint works, as long as we can correctly get the configuration options to the Endpoint instances, which was a problem previously.

I'm also wondering about the role that remote outbound connections have to play in this scenario of a deployment using Wildfly client code to get off the server.

@ropalka
Copy link
Contributor

ropalka commented Jul 10, 2024

@dmlloyd WDYT about this PR proposal?

@dmlloyd
Copy link
Member

dmlloyd commented Jul 10, 2024

Generally I'm in favor of this idea. It's what I always wanted to do but didn't quite manage at the time.

To address (partially) @rachmatowicz's comments:

It is true that XNIO is more centrally configured compared to Remoting. This actually is by design though: it was always intended that one XNIO instance would support the needs of all networking I/O, and this has guided the design of XNIO from the beginning. So XNIO remains a foundational piece, whereas other things (like Remoting) are meant to be added atop it.

As for the loss of per-deployment client configuration, I agree this may end up being a problem in practice. But the solution to this problem is not general (e.g. to replicate just the Endpoint). The better approach would be to determine what configuration makes sense for individual deployments based on the underlying abstractions which are available and the needs of the user. Exposing every possible knob has always been a weakness of this architecture, not a strength. I'd say it's most prudent to address specific problems that are expected on a one-by-one basis, and to handle other problems as they arise, if they arise.

@tadamski tadamski added the hold Do not merge this PR label Jul 11, 2024
@tadamski
Copy link
Contributor Author

I'm putting the issue on hold, because of the ongoing discussion and the need to add tests before the PR is merged.

Copy link

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

@github-actions github-actions bot added the Stale label Aug 26, 2024
Copy link

There has been no activity on this PR for 90 days and it has been closed automatically.

@github-actions github-actions bot closed this Nov 24, 2024
@tadamski tadamski reopened this Jan 8, 2025
@github-actions github-actions bot removed the Stale label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes hold Do not merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants