[jboss-jira] [JBoss JIRA] (WFLY-10577) Undertow subsystem should not depend on clustering code.

Fri Jun 15 09:47:00 EDT 2018

    [ https://issues.jboss.org/browse/WFLY-10577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13591762#comment-13591762 ] 

Paul Ferraro edited comment on WFLY-10577 at 6/15/18 9:46 AM:
--------------------------------------------------------------

{quote}I can understand the dependency to org.wildfly.clustering.service for web cache thingie.
However org.jboss.as.clustering.common does not deal about that. It is an abstraction on top of the Core API, right?{quote}

The wildfly-clustering-service module depends primarily on jboss-msc and jboss-modules, and does not depend on anything from wildfly-core ^1^.  The fundamental abstraction is ServiceConfigurator, which encapsulates the logic for configuring a service, the goal of which is to eliminate code duplication for services built by multiple subsystems.  This module contains interfaces/class originally extracted from wildfly-clustering-common, and is as a dependency of the public API for building singleton services (i.e. org.wildfly.clustering.singleton.api).

The wildfly-clustering-common module (at least the stuff we're talking about) depends primarily on wildfly-clustering-service *and* wildfly-controller (from wildfly-core).  The bulk of this module deals with simplifying subsystem development.  However, there are a couple of interfaces that are specific used by the clustering SPIs that relate to building/configuring services whose dependencies require capability resolution.  These really ought to be split out, either into a separate module (without "clustering" in the name), or (preferably) pulled into wildfly-controller (within core).

\[1] https://github.com/wildfly/wildfly/blob/master/servlet-feature-pack/src/main/resources/modules/system/layers/base/org/wildfly/clustering/service/main/module.xml

{quote}My concern is having subsystem code depends on another one for core features.{quote}

wildfly-clustering-common isn't a subsystem.  It's just logic that was originally common to the clustering subsystems.

{quote}That makes the whole codebase harder to maintain as we have different ways to do the same thing (dealing with MSC Service in that case).{quote}

I understand the concern.  I'll propose a solution at the end of my comments.

{quote}If the Core API (or the MSC Service API) does not provide the correct abstraction (or need another level of abstraction), these features should be done in a place that benefits all subsystems.{quote}

I completely agree.  I proposed migrating some of its core abstractions to wildfly-core a couple of years ago, but we never reached a consensus.

{quote}By the way, the dependency from wildfly-undertow to wildfly-clustering-common is not direct and goes through wildfly-security > wildfly-clustering-infinispan-spi
\[...]
That's why I was not sure if the dependency was intentional or just a byproduct of being available...
{quote}

On the contrary - it's a direct dependency of the SPIs involved in clustering integration with wildfly-clustering-web-undertow.  e.g.
https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/deployment/UndertowDeploymentProcessor.java#L49

Here's what I would propose:

1. Extract the non-deprecated interfaces/classes for configuring services and registering dependencies from wildfly-clustering-service into a separate module, let's call it wildfly-service.  Since this module has no dependencies on any module from wildfly or wildfly-core, this can either reside in a separate maven repo (a la wildfly-common) or within wildfly-core.

2. Extract the interfaces/classes from wildfly-clustering-common appropriate for inclusion into wildfly-controller (within wildfly-core), minimally:
* CapabilityServiceConfigurator (extends ServiceConfigurator adding ability to configure itself from an OperationContext, specifically to resolve names of dependencies provided by capabilities)
* ResourceServiceConfigurator (extends ServiceConfigurator adding ability to configure itself from an OperationContext and Resource model)
* ResourceServiceHandler (encapsulates logic for adding/removing services for a resource - this is logic that is normally duplicated between add/remove operation handlers)
* SimpleResourceServiceHandler (a ResourceServiceHandler that add/removes a single service using a ResourceServiceConfigurator)
* Skeleton add/remove/write-attribute OperationStepHandlers that leverage ResourceServiceHandler
* Binary/UnaryCapabilityNameResolver (common dynamic capability name mapper implementations - to be used in lieu of lambdas)

This would be sufficient to remove the undertow subsystem dependency on wildfly-clustering-common and wildfly-clustering-service.  WDYT?

was (Author: pferraro):
{quote}I can understand the dependency to org.wildfly.clustering.service for web cache thingie.
However org.jboss.as.clustering.common does not deal about that. It is an abstraction on top of the Core API, right?{quote}

The wildfly-clustering-service module depends primarily on jboss-msc and jboss-modules, and does not depend on anything from wildfly-core ^1^.  The fundamental abstraction is ServiceConfigurator, which encapsulates the logic for configuring a service, the goal of which is to eliminate code duplication for services built by multiple subsystems.  This module contains interfaces/class originally extracted from wildfly-clustering-common, and is as a dependency of the public API for building singleton services (i.e. org.wildfly.clustering.singleton.api).

The wildfly-clustering-common module (at least the stuff we're talking about) depends primarily on wildfly-clustering-service *and* wildfly-controller (from wildfly-core).  The bulk of this module deals with simplifying subsystem development.  However, there are a couple of interfaces that are specific used by the clustering SPIs that relate to building/configuring services whose dependencies require capability resolution.  These really ought to be split out, either into a separate module (without "clustering" in the name), or (preferably) pulled into wildfly-controller (within core).

\[1] https://github.com/wildfly/wildfly/blob/master/servlet-feature-pack/src/main/resources/modules/system/layers/base/org/wildfly/clustering/service/main/module.xml

{quote}My concern is having subsystem code depends on another one for core features.{quote}

wildfly-clustering-common isn't a subsystem.  It's just logic that was originally common to the clustering subsystems.

{quote}That makes the whole codebase harder to maintain as we have different ways to do the same thing (dealing with MSC Service in that case).{quote}

I understand the concern.  I'll propose a solution at the end of my comments.

{quote}If the Core API (or the MSC Service API) does not provide the correct abstraction (or need another level of abstraction), these features should be done in a place that benefits all subsystems.{quote}

I completely agree.  I proposed migrating some of its core abstractions to wildfly-core a couple of years ago, but we never reached a consensus.

{quote}By the way, the dependency from wildfly-undertow to wildfly-clustering-common is not direct and goes through wildfly-security > wildfly-clustering-infinispan-spi
\[...]
That's why I was not sure if the dependency was intentional or just a byproduct of being available...
{quote}

On the contrary - it's a direct dependency of the SPIs involved in clustering integration with wildfly-clustering-web-undertow.  e.g.
https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/deployment/UndertowDeploymentProcessor.java#L49

Here's what I would propose:

1. Extract the non-deprecated interfaces/classes for configuring services and registering dependencies from wildfly-clustering-service into a separate module, let's call it wildfly-service.  Since this module has no dependencies on any module from wildfly or wildfly-core, this can either reside in a separate maven repo or within wildfly-core.

2. Extract the interfaces/classes from wildfly-clustering-common appropriate for inclusion into wildfly-controller (within wildfly-core), minimally:
* CapabilityServiceConfigurator (extends ServiceConfigurator adding ability to configure itself from an OperationContext, specifically to resolve names of dependencies provided by capabilities)
* ResourceServiceConfigurator (extends ServiceConfigurator adding ability to configure itself from an OperationContext and Resource model)
* ResourceServiceHandler (encapsulates logic for adding/removing services for a resource - this is logic that is normally duplicated between add/remove operation handlers)
* SimpleResourceServiceHandler (a ResourceServiceHandler that add/removes a single service using a ResourceServiceConfigurator)
* Skeleton add/remove/write-attribute OperationStepHandlers that leverage ResourceServiceHandler
* Binary/UnaryCapabilityNameResolver (common dynamic capability name mapper implementations - to be used in lieu of lambdas)

This would be sufficient to remove the undertow subsystem dependency on wildfly-clustering-common and wildfly-clustering-service.  WDYT?

> Undertow subsystem should not depend on clustering code.
> --------------------------------------------------------
>
>                 Key: WFLY-10577
>                 URL: https://issues.jboss.org/browse/WFLY-10577
>             Project: WildFly
>          Issue Type: Bug
>          Components: Clustering, Web (Undertow)
>            Reporter: Jeff Mesnil
>            Assignee: Paul Ferraro
>
> With WFLY-10352, Undertow subsystem now depends on Clustering code for Clustering's abstraction of the new MSC Service API. For example https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/SingleSignOnSessionFactoryServiceConfigurator.java
> As far as I can tell, the org.jboss.as.clustering.common module is bundled in servlet-feature-pack just so Undertow extension can access its API.
> This is not correct. Undertow should not have to depend on clustering for this kind of abstraction.
> Either these abstractions should be provided by Core or they should not be allowed.

--
This message was sent by Atlassian JIRA
(v7.5.0#75005)