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

Fix for WFLY-12269, transactions galleon layer #12417

Merged
merged 1 commit into from Jul 18, 2019

Conversation

jfdenise
Copy link
Contributor

@jfdenise jfdenise commented Jul 4, 2019

Fix for https://issues.jboss.org/browse/WFLY-12269

  • introduced transactions layer
  • replaced inclusion of transactions-all feature-group by transactions layer dependency in layers depending on transactions.
  • added test.

@wildfly-ci wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 4, 2019
<?xml version="1.0" ?>
<layer-spec xmlns="urn:jboss:galleon:layer-spec:1.0" name="transactions">
<dependencies>
<layer name="web-server"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfdenise Does this need to depend on this aggregation layer, as opposed to specific detail layers from servlet f-p?

There are a few in this category. Here's what depends on web-server:

$ git grep "layer name=\"web-server\""
galleon-pack/src/main/resources/layers/standalone/cdi/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/h2-database/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/jaxrs/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/jms-activemq/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/jpa/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/observability/layer-spec.xml:        <layer name="web-server"/>
galleon-pack/src/main/resources/layers/standalone/resource-adapters/layer-spec.xml:        <layer name="web-server"/>

Obviously jaxrs needs web-server. The others it's less obvious that HTTP is required.

The advantage of doing it this way is for a servlet app with jpa support you can just say "jpa" instead of "web-server, jpa". And perhaps for compatibility reasons we have to leave it that way for the existing layers. But that doesn't apply to this 'transactions' one, since it is new.

Apologies if we've discussed this and I've forgotten. The wildfly-dev "Wildfly layers" doesn't have any discussion, other than your initial post that stated it would work this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was that web-server would be the minimal layer any "full feature (cdi, ...)" would require. Making the layer "runnable" and reachable from http.
transactions only requires elytron, so we could define the layer to only depend on elytron, but doing so it would have to be combined with other full features (if used outside cloud-profile or other layers that depend on it).
transaction is perhaps not a feature you add alone in your config, it is a dependency for top level layers. I agree that having it depend on elytron should be sufficient. It should be still runnable and you could combine it with smaller layers.
I would be ready to revisit them all and change documentation. They are not yet supported so changes can occur.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfdenise A question then would be whether we would provide other layers that depend on transactions but which don't also involve an a servlet container.

Not just whether they might exist in theory, but whether we would provide them. If we don't provide them, then the user who would want that would be somehow doing fairly complex provisioning config manipulation anyway, and excluding the servlet container / undertow parts would just be another part of that. Not unreasonable.

So what layer that would use transactions would we provide that doesn't have an HTTP endpoint? It would have to be something that involves some other remote access protocol (e.g. native remoting or IIOP or QPID.) Or something that doesn't involve listening for outside connections at all.

For the 'other remote access protocol' case, I don't see us providing such a layer. Our standard configs aim toward having a single HTTP socket for application use with HTTP Upgrade used if needed.

The "doesn't involve listening for outside connections at all" case is valid though. A server set up for batch jobs. Or a server that runs MDBs, where the messages processed by the MDB are pulled from a remote broker over a client socket, not a server socket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know with an @ ping or chat what you want to do about this one. This discussion is broader than just this layer but I don't want to block this while we think about the other ones listed in my earlier comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bstansberry , now that I see these "mini aggregations" of web-server+feature, it looks wrong. We are loosing some flexibility to make layers "simpler" to use. Having to express web-server in some cases is not that difficult and makes sense.
For future layers definition it would create, as you noted, some un-necessary complexity.

This transactions layer should depend on elytron+ee (it needs it in order to set use-transaction-setup-provider).

I would be ready to open another JIRA and update layers and doc with the removal of web-server for all layers that don't 100% require a web-server.
If we keep the web-server dependency for jaxrs, we could possibly rename it to be "jaxrs-server" that would be the aggregation, jaxrs layer would not depend on web-server (for very exotic case). That would seem more homogeneous.

We would keep the rule that all layers identified to be used from user configuration should be provisioned and runnable alone (even if they don't offer any valid feature).

@jfdenise
Copy link
Contributor Author

jfdenise commented Jul 17, 2019

@bstansberry , I forced push the PR, transaction should be good to go. I opened https://issues.jboss.org/browse/WFLY-12299 for other layers.

@bstansberry bstansberry merged commit d831ed3 into wildfly:master Jul 18, 2019
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
Projects
None yet
3 participants